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

Find nearest color JS #2356

Closed
wants to merge 45 commits into from
Closed

Conversation

Holzhaus
Copy link
Member

Initial attempt to add lowest-distance color matching for hotcues to Mixxx (as disscussed in #2119).
This is a follow up to #2345.

Demo: https://www.youtube.com/watch?v=fj9HUyNjJAA

.. instead of PredefinedColorPointer.

COs and the database now hold the color hex int representation instead of a color id.
and change cue color CO name
@daschuer
Copy link
Member

Thank you for taking care.
I think you should open this PR against @ferranpujolcamins repro.

@Swiftb0y
Copy link
Member

While this approach is certainly usable. I'd much rather have the API I previously outlined on Zulip in 2.3 included. I'll see how much I can do over the weekends.

@daschuer
Copy link
Member

The code looks good, btw.

@Holzhaus
Copy link
Member Author

Holzhaus commented Nov 14, 2019

@daschuer Thanks, I opened the PR here to have more visibility, so that we can discuss the API. I'll close this and open the PR to @ferranpujolcamins Repository when it's ready.
@Swiftb0y I saw the API proposal on Zulip, however there are a few problems or questions regarding it:

  1. Why do we need to register/deregister colors? Is there any benefit from this except to make caching easier because we know the set of supported colors?
  2. Is the color registration/caching code supposed to be implemented in JS or in C++?
    We could easily add a JS caching solution on top of this implementation, but I don't know if it's worthwhile because the color distance code should be quite fast.
  3. Is the color instance shared between all controller scripts? If so, the there could be problems when mutliple controller scripts register different colors and the code returns a color for the wrong controller. According to @ferranpujolcamins this shouldn't be a problem.
  4. A controller might support different sets of colors (e.g. performance pads support 16 colors, dedicated loop section with just 5 colors). This is hypothetical, but wouldn't be covered by the API proposal on Zulip.

@Swiftb0y
Copy link
Member

Why do we need to register/deregister colors? Is there any benefit from this except to make caching easier because we know the set of supported colors?

Yes, its primary caching purpose is for caching because I think transforming a js Map/Array into a QColor >8 times every single time we want to transform the color might be a bit wasteful of performance I also don't really see a usecase which benefits in being able to provide a different palette on every single lookup..

Is the color registration/caching code supposed to be implemented in JS or in C++?
We could easily add a JS caching solution on top of this implementation, but I don't know if it's worthwhile because the color distance code should be quite fast.

I would have done it in C++ because of the previously mentioned performance concerns.

A controller might support different sets of colors (e.g. performance pads support 16 colors, dedicated loop section with just 5 colors). This is hypothetical, but wouldn't be covered by the API proposal on Zulip.

This is considered by the second proposal on Zulip because it doesn't declare a namespace but a class instead. So you could create two different instances of the color object with different maps in the same script.

Is the color instance shared between all controller scripts? If so, the there could be problems when mutliple controller scripts register different colors and the code returns a color for the wrong controller. According to @ferranpujolcamins this shouldn't be a problem.

As I said previously, ColorMapper in the API is a class which can be instanced so even if didn't have different QJSProxy's there still wasn't any state to be shared.


The optimization concerns might be invalid but with the current state, I am missing the type flexibility I proposed in the previous API.

@Holzhaus
Copy link
Member Author

Why do we need to register/deregister colors? Is there any benefit from this except to make caching easier because we know the set of supported colors?

Yes, its primary caching purpose is for caching because I think transforming a js Map/Array into a QColor >8 times every single time we want to transform the color might be a bit wasteful of performance I also don't really see a usecase which benefits in being able to provide a different palette on every single lookup..

Agreed. Is there any use case for deregistering colors though? If not, we could just pass the colors in the object constructor and get rid of register/deregister functions. That would also mean we don't need to bother with the possibility that somebody registers or unregisters a color at runtime. Otherwise we'd need to check that and invalidate the cache.

Is the color registration/caching code supposed to be implemented in JS or in C++?
We could easily add a JS caching solution on top of this implementation, but I don't know if it's worthwhile because the color distance code should be quite fast.

I would have done it in C++ because of the previously mentioned performance concerns.

The question is how costly the JS <-> C++ context switch is. I have no idea, but it could actually make it worse.

A controller might support different sets of colors (e.g. performance pads support 16 colors, dedicated loop section with just 5 colors). This is hypothetical, but wouldn't be covered by the API proposal on Zulip.

This is considered by the second proposal on Zulip because it doesn't declare a namespace but a class instead. So you could create two different instances of the color object with different maps in the same script.

Ok.

@Be-ing
Copy link
Contributor

Be-ing commented Nov 14, 2019

This approach requires scripts to have their own way of associating colors with MIDI codes (or other values). With the API @Swiftb0y proposed, the ColorMapper class would take care of that.

Regarding performance, I don't think it's a big issue. However, for 2.3 we have no Map in JavaScript, but we do have QMap in C++.

@Holzhaus
Copy link
Member Author

Ok, I added a ColorMapper class for usage in controller scripts. This takes care of storing/looking up values in the cache and calculating distance if there's a cache miss. I had to use QRgb instead of QColor, because you apparently can't use it as a QMap key. We QRgb we can possibly even switch to QHash which should be even faster.

I didn't add register/deregister functions and put everything into the constructor, because I couldn't think of a use case and didn't want to mess around with cache invalidation. But if there's need for it, we can add it.

Let me know what you think.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

I'm not well versed enough in C++ to comment about the details but it looks good from a high-level point of view.

@Holzhaus
Copy link
Member Author

Ok, if there aren't any further comments I'll close this and open a PR for @ferranpujolcamins repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants