Skip to content

Conversation

@Jonarw
Copy link
Member

@Jonarw Jonarw commented Nov 6, 2023

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:

  • Implement LogarithmicColorAxis
  • Clean up Color Axis examples and move Palette examples to a separate category
  • Make AxisRendererBase generic - this is a breaking change for any derived classes, but one that is easily fixed. And really it should always have been generic.
  • Bump LangVersion where needed

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

@oxyplot/admins

Copy link
Contributor

@VisualMelon VisualMelon left a 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.

@Jonarw
Copy link
Member Author

Jonarw commented Nov 14, 2023

Thanks for pointing out my just slightly embarrassing mistakes ;)

@VisualMelon
Copy link
Contributor

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.

@Jonarw
Copy link
Member Author

Jonarw commented Jan 8, 2024

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

@VisualMelon
Copy link
Contributor

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.
Copy link
Contributor

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
Member Author

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?

Copy link
Contributor

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.

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.

LogarithmicColorAxis

2 participants