Skip to content
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.

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

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

Merged
merged 2 commits into from
Jan 25, 2025

Conversation

kaixiong
Copy link
Member

This PR implements:

  • standard C++ container methods such as begin() and end() for iteration
  • equality and inequality operators for comparing palettes

@kaixiong kaixiong self-assigned this Jan 23, 2025
@kaixiong kaixiong requested a review from hartwork January 23, 2025 21:48
@@ -35,6 +35,7 @@

#ifdef __cplusplus

#include <algorithm>
Copy link
Member

Choose a reason for hiding this comment

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

I'm likely missing something, but I do not see anything in the first commit that would need <algorithm> and also not in the second. What is <algorithm> used for/by in the pull request?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, it's not used. I think I missed this when splitting the commit out.

using const_iterator = std::vector<Color>::const_iterator;
using iterator = std::vector<Color>::iterator;
Copy link
Member

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

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

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

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.

@kaixiong kaixiong force-pushed the add-palette-iteration-comparison-api branch from 1788dec to a530883 Compare January 25, 2025 18:25
@kaixiong kaixiong merged commit b521806 into master Jan 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants