oxyplot/oxyplotPublic

NotificationsYou must be signed in to change notification settings
Fork983
Star3.5k

Comments

Implement LogarithmicColorAxis#2049

Merged

Jonarw merged 4 commits intooxyplot:developfrom

Jonarw:logcoloraxis

Feb 5, 2024

Merged

Implement LogarithmicColorAxis#2049
Jonarw merged 4 commits intooxyplot:developfrom
Jonarw:logcoloraxis

Conversation

Copy link

Member

Jonarw commentedNov 6, 2023•
edited
Loading

Fixes#92.

Checklist

I have included examples or tests
I have updated the change log
I am listed in the CONTRIBUTORS file
I have cleaned up the commit history (use rebase and squash)

Changes proposed in this pull request:

ImplementLogarithmicColorAxis
Clean up Color Axis examples and move Palette examples to a separate category
MakeAxisRendererBase generic - this is a breaking change for any derived classes, but one that is easily fixed. And really it should always have been generic.
BumpLangVersion where needed

The reason for the slightly more wide-ranging code changes is as follows:
Until now we had 3IColorAxis implementations:LinearColorAxis,CategoryColorAxis andRangeColorAxis, which duplicated a lot of code between them. AddingLogarithmicColorAxis would have essentially duplicated most ofLinearColorAxis yet again. Therefore I decided to move the color axis rendering functionality into aColorAxisRenderer, so it can be shared between theIColorAxis implementations.

@oxyplot/admins

Jonarw added2 commits

November 6, 2023 09:57

Implement LogarithmicColorAxis

57fca9d

Update CHANGELOG.md

95debc0

Jonarw commented

Nov 6, 2023

View reviewed changes

Source/OxyPlot/Utilities/Helpers.csShow resolvedHide resolved

fix test

b9b2424

VisualMelon requested changes

Nov 11, 2023

View reviewed changes

Copy link

Contributor

VisualMelon left a comment•
edited
Loading

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/OxyPlot/Utilities/Helpers.csShow resolvedHide resolved

Source/OxyPlot/Axes/Rendering/RangeColorAxisRenderer.cs OutdatedShow resolvedHide resolved

Source/OxyPlot/Axes/Rendering/ColorAxisRenderer.cs OutdatedShow resolvedHide resolved

Source/OxyPlot/Axes/Rendering/CategoryColorAxisRenderer.cs OutdatedShow resolvedHide resolved

Source/Examples/ExampleLibrary/Misc/PaletteExamples.cs OutdatedShow resolvedHide resolved

Source/Examples/ExampleLibrary/Axes/LogarithmicColorAxisExamples.cs OutdatedShow resolvedHide resolved

CHANGELOG.md OutdatedShow resolvedHide resolved

fix copy&paste mistakes

25962e7

Copy link

MemberAuthor

Jonarw commentedNov 14, 2023

Thanks for pointing out my just slightly embarrassing mistakes ;)

Copy link

Contributor

VisualMelon commentedNov 14, 2023

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.

Copy link

MemberAuthor

Jonarw commentedJan 8, 2024

Hi@VisualMelon! Did you have time to take another look at this by any chance?

Copy link

Contributor

VisualMelon commentedJan 8, 2024

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 ;)

VisualMelon reviewed

Jan 8, 2024

View reviewed changes

Source/OxyPlot/Axes/LogarithmicAxis.csShow resolvedHide resolved

VisualMelon reviewed

Jan 8, 2024

View reviewed changes

Source/OxyPlot/Axes/Rendering/NumericColorAxisRenderer.cs

		}

		/// <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.

Copy link

Contributor

VisualMelonJan 8, 2024

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

Copy link

MemberAuthor

JonarwJan 10, 2024

Choose a reason for hiding this comment

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

Even forLogarithmicColorAxis, 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 happenselsewhere. This is why we have to use this linear transformation here. Does this make sense?

Copy link

Contributor

VisualMelonJan 10, 2024

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.