Closed astrofrog closed 5 years ago
A few comments:
ColorMapper
, maybe? I am not a big fan of having a tense-only distinction between two class names, but I'm having trouble thinking of other ideas besides equally-unwieldy things like ColorMapData
, etc.ColorMap
member is not public
— my guess is that was just an oversight.ColorMapMode
, however, to avoid gratuitous differences with the Windows codebase. I am going to take a look at this. In general we should try to not break old code, but if we handcuff our self too much then we can never move forward. We should also port many of these changes and fixes to the Windows Release as well.
After I review it we can have a conference call and figure out a way to deal with this class of fixes/improvements.
@pkgw - thanks for the feedback! I renamed DataColorMap
to ColorMapper
following your good suggestion. I'll wait to make colorMap public until @astrojonathan has taken a look in case there is a good reason it isn't public.
@astrojonathan - good idea about the conference call - I agree it would be good to figure out a way to move forward with these kinds of changes.
I've managed to successfully get this to work and result in tour files being created that are compatible with the Windows client:
At the moment, this does not allow completely custom colormaps, because these can't yet easily be serialized. Instead I've implemented a few common colormaps from Matplotlib. I've also included Python code (in a comment) to add more colormaps if we are ok with this approach.
@pkgw @astrojonathan - this is now ready for review/merging (we can always add more colormaps, e.g. diverging ones, in a future PR)
@pkgw - I've renamed the file and now validate the colormap name (good catch!). Due to time zone weirdness on my PC, the commit appears above your comment but I did push a new commit (direct link: https://github.com/WorldWideTelescope/wwt-web-client/pull/238/commits/fba0bf4627d6fd50d3da1320c3e2d6aa5ca1c55a)
In a similar vein to #237, this adds support for doing color-mapping between values and colormaps straight in the SDK, with a pre-normalization of the form:
applied beforehand. In many cases, astronomers don't have tables with point sizes already in the table - instead they will have some value, e.g. mass or temperature and will want to map these to actual colors. Rather than have to re-generate the table many times, the attributes here allow normalization to be turned on in the SDK, and to control the min/max, as well as the colormap used.
This will allow us to then implement a much more efficient approach for WorldWideTelescope/pywwt#138 in PyWWT - at the moment, we constantly re-generate data tables and send them from Python to JS - with this, we'll be able to just modify the min/max and the colormap dynamically.
Example usage in the JS SDK:
A few notes:
For now this adds an option to the
ColorMaps
enum, but based on feedback from @astrojonathan this would break compatibility with the desktop client, so it might make more sense to have a boolean attribute to turn this behavior on/off?I'm not too happy with the naming - ideally we'd just use
ColorMap
for the colormap, but this is already used to mean the color map mode to use, hence the convoluted terminology in this PR. I'm open to suggestions though, and I'll have a think too.At the moment
set__colorMap
is private, but it might make sense to make this public? In fact, since it was private, maybe we can change the API to make itset_colorMapMode
and then makeset_dataColorMap
intoset_colorMap
?The colormap class is very simple, on purpose. The idea is that it doesn't do any interpolation but instead just divides the colormap into as many bins as there are colors and checks which bin values (in the range[0:1]) fall in. The idea is that this then allows us to implement discrete colormaps, and continuous ones are then just made up of colormaps with 256 or more colors. Interpolation in RGB space is tricky, so it's better to let other tools handle that and just pass the interpolated colormap here.