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

Color palettes #289

Merged
merged 79 commits into from
Dec 11, 2024
Merged

Color palettes #289

merged 79 commits into from
Dec 11, 2024

Conversation

justinlaughlin
Copy link
Contributor

@justinlaughlin justinlaughlin commented Jul 11, 2024

Refactor Palettes for better customization

This PR:

  • Provides a more flexible implementation for color palettes, primarily to allow specification of palettes both at compile and run time.
  • Compile time palettes are defined under lib/base_palettes.cpp and exported as BasePalettes. Some of these were generated by functions and are now just defined explicitly.
  • Run time palettes can be specified using the command line argument -p <palette_filename>; an example palette file with the proper structure is under share/palettes.txt (this includes all of the palettes mentioned in the old version of the PR).
  • Adds a few structs/classes
    • Struct RGBAf (to represent RGBA in [0,1] float format)
    • Struct Palette (holds vector<RGBAf> colors and string name)
    • Class PaletteRegistry (holds vector<shared_ptr<Palette>>)
  • Updates PaletteState; previously it required the global variables Num_RGB_Palettes, RGB_Palette_Sizes, RGB_Palettes, and RGB_Palette_Names. Now it uses a pointer to a PaletteRegistry. By default, a PaletteState will be initialized with the global variable BasePalettes but it is not necessary.

Misc notes:

  • Palettes can be loaded with explicit alpha channels now; right now this doesn't do anything. I think this would require some changes to PaletteState::GenerateAlphaTexture.
  • Right now because BasePalettes is updated before any PaletteState are created, they initialize properly, but because textures are created by PaletteState::Init this could lead to problems if palettes are loaded in at a later point in the run. Moving the functionality to Palette, eg Palette::as_texture() with caching could be a solution. (there are several other methods that should be moved to Palette so that PaletteState is just in charge of the state).
  • Palettes can now be retrieved by name but this isn't used anywhere yet.

Original PR

There is a nice resource from Fabio Crameri that provides a lot of color-vision deficiency friendly and perceptually-uniform color maps [see issue 268, and reference]. This PR adds most of those colormaps into GLVis.

Here is an example of batlow

I wrote a small script to help munge the colormap files into something copy-pasteable. I'm not sure if it's worth adding to the repo but I posted it as a gist. The colormaps added in this PR are listed in the gist:

cmnames = ['batlow', 'batlowW', 'batlowK', 'glasgow', 'lipari', 'navia', # continuous
           'oleron', 'bukavu', 'fes', # multi-sequential
           'hawaii', 'buda', 'imola', # discrete
           'oslo', 'nuuk', 'lajolla', 'bamako', 'davos', # categorical
           'bilbao', 'lapaz', 'acton', 'turku', 'tokyo',
           'broc', 'cork', 'vik', 'lisbon', 'tofino', 'berlin', # diverging
           ]

@justinlaughlin
Copy link
Contributor Author

It looks like tests 15 and 17 are failing because they cycle color palettes backwards using P, so they get around to the new palettes. Maybe we should restrict palettes that cycle to a subset of the total? Or update tests?

@v-dobrev
Copy link
Member

This seems like going a bit overboard with the palettes. 😁 The size of palettes.cpp is essentially doubled in terms of lines!

@justinlaughlin
Copy link
Contributor Author

I started with just a few, but it was taking a while so I wrote a script to help, and then I figured might as well add most 😆. I think we had 43 before and this adds 28? Could pare it down too...

@justinlaughlin
Copy link
Contributor Author

Some suggestions from gm:

  • Add option for user to point to custom colormap file
  • Keep "cyclable" colormaps the same for backwards compatability

Extra colormaps + user defined ones would still be accessed through F6?

@tzanio tzanio mentioned this pull request Jul 17, 2024
22 tasks
@tzanio
Copy link
Member

tzanio commented Jul 17, 2024

To clarify

Add option for user to point to custom colormap file

The idea will be to have an optional colormap file (could be configurable at build time, or just "colormaps.dat" in the same directory as the executable) and for glvis to check for that file at startup and if found to add the additional colormaps at the end of the default list.

This way, we preserve the default behavior, while allowing for local customization.

@tzanio
Copy link
Member

tzanio commented Oct 15, 2024

@justinlaughlin, I think it will be still cool to add an optional input file for this. Happy to chat if you want to give it a try.

lib/palettes_base.cpp Outdated Show resolved Hide resolved
lib/palettes_base.cpp Outdated Show resolved Hide resolved
lib/palettes_base.hpp Outdated Show resolved Hide resolved
lib/palettes_base.cpp Outdated Show resolved Hide resolved
lib/palettes.cpp Outdated Show resolved Hide resolved
lib/palettes.cpp Outdated Show resolved Hide resolved
lib/palettes.cpp Outdated Show resolved Hide resolved
lib/palettes.cpp Outdated Show resolved Hide resolved
lib/palettes.cpp Outdated
{
if (!first_init)
if ((int)(textures.size()) != Palettes->NumPalettes())
Copy link
Contributor

@najlkin najlkin Dec 6, 2024

Choose a reason for hiding this comment

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

This is unfortunately insufficient check, because the number might be the same, but the palettes might be different 💣 . I do not see an easy way how to do this properly, so it is fine for the moment, I guess, but we need to keep this in mind when runtime loading of palettes happens and the driving code will have to call InitTextures() directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not used at the moment but you're right. I changed it to a more generic reinitialize argument that can be used in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense too 👍 . Maybe let us keep this open for future reference.

lib/palettes_base.hpp Show resolved Hide resolved
lib/palettes.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@najlkin najlkin left a comment

Choose a reason for hiding this comment

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

Almost almost there 😄 Pls, do not forget about that loading above.

@justinlaughlin
Copy link
Contributor Author

Added some changes based on your feedback. Thanks!

break;
cout << "Warning: palette name <" << palname
<< "> already exists. Overriding..." << endl;
palettes.erase(palettes.begin() + idx);
Copy link
Contributor

Choose a reason for hiding this comment

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

As it is a good practice of premature optimization in MFEM, it would be more optimal if you do palettes[idx] = as_unique<Palette>(palname) 😄 . But for unification of the approach it is better like this. Would be better to wrap this call in a method, but also not super important.

@justinlaughlin
Copy link
Contributor Author

Should this be squashed before merging? There is a lot of commits.

Copy link
Member

@tzanio tzanio left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this improvement, @justinlaughlin and @najlkin !

@tzanio tzanio merged commit 5813267 into master Dec 11, 2024
10 checks passed
@tzanio tzanio deleted the more-color-palettes branch December 11, 2024 00:53
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.

4 participants