-
Notifications
You must be signed in to change notification settings - Fork 987
Implement LogarithmicColorAxis #2049
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite got my head around the architecture changes, but everything looks good to me in isolation and results look good too so I'm basically happy once the really boring comment changes are fixed.
Source/Examples/ExampleLibrary/Axes/LogarithmicColorAxisExamples.cs
Outdated
Show resolved
Hide resolved
|
Thanks for pointing out my just slightly embarrassing mistakes ;) |
|
I'm sure no-one ever looks at those comments; it's only because in the github view it's always right next to the filename that I notice ;) Will give this another look a bit later this week. |
|
Hi @VisualMelon! Did you have time to take another look at this by any chance? |
|
I didn't get around to it, but I should have time later today or tomorrow, so complain at me if I've not got back to you in 36 hours ;) |
| } | ||
|
|
||
| /// <summary> | ||
| /// Transforms a value to a screen position. We don't use the regular Transform functions of the axis here, as the color block should always be drawn with linear scaling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this makes sense; will have to come back to this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even for LogarithmicColorAxis, we want the the color transition to be drawn in linear space, as it would not be very readable in logarithmic.
The transformation to log space happens elsewhere. This is why we have to use this linear transformation here. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly sounds correct, but something felt fishy to me so I'd like to take another look when I have a bit more time. Happy for this to be merged in the interim.
Fixes #92.
Checklist
Changes proposed in this pull request:
LogarithmicColorAxisAxisRendererBasegeneric - this is a breaking change for any derived classes, but one that is easily fixed. And really it should always have been generic.LangVersionwhere neededThe reason for the slightly more wide-ranging code changes is as follows:
Until now we had 3
IColorAxisimplementations:LinearColorAxis,CategoryColorAxisandRangeColorAxis, which duplicated a lot of code between them. AddingLogarithmicColorAxiswould have essentially duplicated most ofLinearColorAxisyet again. Therefore I decided to move the color axis rendering functionality into aColorAxisRenderer, so it can be shared between theIColorAxisimplementations.@oxyplot/admins