Skip to content

Add LV::Palette methods for iterations and comparisons #384

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 25, 2025
Merged

Add LV::Palette methods for iterations and comparisons #384

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d0f64b
Core (LV::Palette): Add comparison operators.
kaixiong Jan 23, 2025
a530883
Core (LV::Palette): Add methods for iteration.
kaixiong Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions libvisual/libvisual/lv_palette.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ namespace LV {
//!
struct LV_API Palette
{
using const_iterator = std::vector<Color>::const_iterator;
using iterator = std::vector<Color>::iterator;
Comment on lines +48 to +49
Copy link
Member

@hartwork hartwork Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While these two seem to help readability further down in some regard, the fact that the color aspect — a key aspect of the return type — is hidden away seems (worse than hiding the vector away) and like a disservice to me. Maybe const_color_iterator? Is there a concept other than D.R.Y. and brevity here that I am missing?

Copy link
Member Author

@kaixiong kaixiong Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type erasure is kind of the point for generics. These type definitions are part of LV::Palette's interface for generic programming (think protocols in Python duck typing) and are standard in C++ containers.

For example, imagine a generic function that works on some sequence of LV::Color objects. Say, computing the average colour intensity of the collection. The algorithm doesn't know or want to know if it's a std::vector<Color>, a std::array<Color>, a C array (LV::Color *), LV::Palette or some fancy structure that does size optimization tricks. All it cares about is iterating, calculate each color's intensity, accumulating and finally averaging in the end.

So in that function, when it needs to declare and instantiate an iterator for the container, it can declare Container::iterator, Container::const_iterator as its type, where Container could be any of the above.

Copy link
Member

@hartwork hartwork Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaixiong by Container you mean Palette I assume? I think I understand where you are going with this. I will approve the pull request and only removal of <algorithm> remains to be changed…

Copy link
Member Author

@kaixiong kaixiong Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartwork, hmm upon some more thinking, perhaps I should also provide typedefs for value_type (= Color) and maybe reference (Color &) and const_reference (Color const&). This also serves as documentation what the element type is when iterating over an LV::Palette instance. I don't really want to implement the full SequenceContainer interface just yet though.


std::vector<Color> colors;

/**
Expand All @@ -70,6 +73,46 @@ namespace LV {
return *this;
}

constexpr friend bool operator== (Palette const& lhs, Palette const& rhs)
{
return lhs.colors == rhs.colors;
}

constexpr friend bool operator!= (Palette const& lhs, Palette const& rhs)
{
return !(lhs == rhs);
}

constexpr const_iterator cbegin () const noexcept
{
return colors.cbegin ();
}

constexpr const_iterator cend () const noexcept
{
return colors.cend ();
}

constexpr const_iterator begin () const noexcept
{
return colors.begin ();
}

constexpr const_iterator end () const noexcept
{
return colors.end ();
}

constexpr iterator begin () noexcept
{
return colors.begin ();
}

constexpr iterator end () noexcept
{
return colors.end ();
}

bool empty () const
{
return colors.empty ();
Expand Down