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

RemoveColormap API function added #344

Closed
wants to merge 2 commits into from

Conversation

ToppDev
Copy link

@ToppDev ToppDev commented Mar 26, 2022

Following up on #341 I implemented the feature.

There are 2 issues I would like your input with:

  1. Where or how would you define the number of built-in colormaps? I just defined it as IMPLOT_CMAP_COUNT_BUILT_IN directly above the Initialize function. However there might be a better place to put it, to fit into the library structure.
  2. I actually don't remove the respective entry out of the tables, but rather call the RebuildTables function at the end. That was kinda out of laziness from my side. If you prefer it, I can still implement a void _RemoveTable(ImPlotColormap cmap) function which only removes the needed entries.

@ToppDev
Copy link
Author

ToppDev commented Jun 17, 2022

any chance to get this into the master branch? (then I could put my submodules on the github master again instead of keeping my own repo)

@epezent
Copy link
Owner

epezent commented Jun 19, 2022

Hey @ToppDev -- so sorry I've let this go unattended for this long. I'm ready to get this merged. There's one issue, however. As it is currently implemented, the current ImPlotStyle::Colormap index or any user cached ImPlotColormap indices (i.e. return values of AddColormap) are invalidated if a colormap with a lower index is removed. You can see this in the GIF below, where the current ImPlotStyle::Colormap index refers to RGB initially but then CMYK once Dracula is removed.

cmap

I have two possible solutions:

  1. ImPlotColormaps become hashed IDs instead of indices. We would need to change around the internals of ImPlotColormapData to use ImPool or some other hash based storage container where we currently use ImVector. We'd also need to figure out how to handle the built-in enums which are currently hard coded indices. And we'd need a new unsigned sentinal value for specificying the "current" colormap in our public API (we currently use IMPLOT_AUTO which is -1).

  2. We keep the ImVector storage containers but instead of erasing their elements when a colormap is removed, we mark that index as "available". When a new colormap is added, we search for the next available index. If one exists, we reuse it, otherwise we added new elements to the vectors. This may make the Count variable less useful, and we'd need to check if an index is used where we iterate the colormap vectors (e.g. inside of the colormap style editor and metrics windows).

I think I am leaning toward (2), but I'm curious to hear your thoughts.

@ToppDev
Copy link
Author

ToppDev commented Jul 13, 2022

Hey, sorry for also coming back late at you.
Thanks for bringing this issue to my attention, I somehow missed it.

I also think your second solution sounds better. While the first solution is probably cleaner, it would affect a lot of code. It also would break the compatibility to e.g. IMPLOT_AUTO as you mentioned and even if we choose a different value, it would be not as intuitive for the user I guess.

I will try implementing the second solution till next week and see how much code is affected by it

@ToppDev ToppDev closed this Aug 8, 2022
@ToppDev ToppDev reopened this Aug 8, 2022
@ToppDev
Copy link
Author

ToppDev commented Aug 8, 2022

so I looked into the problem some more.
I think I underestimated the drawback of the Count variable being less useful as you mentioned earlier. It basically introduces all kind of inconsistencies which have to be accounted for and this bloats up the code.

Therefore I actually wondered if it is the task of the library to ensure, that if the user decided to store a colormap index, that the index keeps the same. I made a commit so that the Style Editor correctly decrements the gp.Style.Colormap variable.
However I think it is the users responsibility to decrement his variables. Wrong usage of the API is anyway protected by all the IM_ASSERT_USER_ERROR checks.

What do you think about this?

@epezent
Copy link
Owner

epezent commented Sep 14, 2022

However I think it is the users responsibility to decrement his variables.

I think this would be a bad choice and would cause several headaches down the road. I am stilll in favor of (2) described above, or not supporting this particular feataure until we have a better alternative.

As I imagine the typical need for RemoveColormap is to replace an existing colormap with new colors, what if we instead provided a mechanism to "update" a colormap or replace colors in an already existing colormap?

@epezent
Copy link
Owner

epezent commented Sep 14, 2022

Somewhat to this, I recently had the idea that it would be neat if a reversed colormap could be specificied by negating the value of a Colormap index, e.g. PushColormap(-ImPlotColormap_Viridis). Upon receving a neative index, we would still use the positive index to for array indexing purposes, but we'd set a flag to instruct ImPlot to interpolate the map in reverse.
This would require that all colormap indices be greater than 0, so we'd use 0 as our "current" sentinal value isntead of IMPLOT_AUTO=-1.

1 similar comment
@epezent
Copy link
Owner

epezent commented Sep 14, 2022

Somewhat to this, I recently had the idea that it would be neat if a reversed colormap could be specificied by negating the value of a Colormap index, e.g. PushColormap(-ImPlotColormap_Viridis). Upon receving a neative index, we would still use the positive index to for array indexing purposes, but we'd set a flag to instruct ImPlot to interpolate the map in reverse.
This would require that all colormap indices be greater than 0, so we'd use 0 as our "current" sentinal value isntead of IMPLOT_AUTO=-1.

@ToppDev
Copy link
Author

ToppDev commented Jun 16, 2023

Sorry to also leave this unattended for so long.

I do not really have time to continue this, as it become a bigger problem than I anticipated at the start.
So if anyone wants to take over my work, feel free. Otherwise I would just close this PR.

Thanks for your guidance though @epezent

@ToppDev ToppDev closed this Jun 16, 2023
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.

None yet

2 participants