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

Remove radius1 for regular shapes, use radius instead #15191

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented Sep 28, 2023

Having two optional parameters where one is required is bad for type-checking.

Current handling is inconsistent. RegularShape constructor treated radius and radius1 as aliases while the webgl style parser used radius exclusively for polygons and radius1 exclusively for stars.

@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-15191--ol-site.netlify.app/.

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

I really like this simplification, thanks! And as we are changing property names anyway, could we use something like radiusInner instead of radius2?

@@ -1,5 +1,10 @@
## Upgrade notes

### Next release
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should explicitly mark that as a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the upgrade notes all about breaking changes? I don't see any previous changes marked explicitly as breaking there.
What do you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

For version 8 the breaking changes were put in separate section: https://github.com/openlayers/openlayers/blob/main/changelog/upgrade-notes.md#backwards-incompatible-changes

This is what I had in mind

@MoonE
Copy link
Contributor Author

MoonE commented Sep 29, 2023

For RegularShape there is no requirement for radius to be larger than radius2. So calling it inner / outer doesn't make sense there.
For webgl this unfortunately doesn't hold true, but if this is improved upon in the future it would be a misnomer.
See https://openlayers.org/en/latest/examples/webgl-points-layer.html with a style like:

{
  "shape-points": 5,
  "shape-radius1": ["+", ["*", ["sin", ["time"]], 10], 20],
  "shape-radius2": ["+", ["*", ["sin", ["time"]], -10], 20],
  "shape-rotation": ["*", ["time"], 0.2],
  "shape-fill-color": "red",
  "shape-stroke-color": "black",
  "shape-stroke-width": 2
}

@MoonE
Copy link
Contributor Author

MoonE commented Sep 29, 2023

I added a commit that allows radius2 to be larger than radius for webgl rendering.

@MoonE MoonE force-pushed the regularshape-remove-radius-1 branch 3 times, most recently from 9958a0a to 07fce62 Compare September 29, 2023 19:28
@jahow
Copy link
Contributor

jahow commented Sep 30, 2023

For RegularShape there is no requirement for radius to be larger than radius2. So calling it inner / outer doesn't make sense there. For webgl this unfortunately doesn't hold true, but if this is improved upon in the future it would be a misnomer

Thanks, you're totally right!

Copy link
Contributor

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thanks for the simplification and fixing the webgl style parser in the process! I'm not sure when would be the best time for merging this though, but still, at least this is approved.

@MoonE MoonE mentioned this pull request Nov 9, 2023
@MoonE MoonE force-pushed the regularshape-remove-radius-1 branch from 07fce62 to cad98c2 Compare November 9, 2023 21:11
Having two optional parameters where one is required is bad for type-checking.

Current handling is inconsistent. RegularShape constructor treated radius and
radius1 as aliases while the webgl style parser used radius exclusively for
polygons and radius1 exclusively for stars.
@MoonE MoonE force-pushed the regularshape-remove-radius-1 branch from cad98c2 to 2965df1 Compare November 10, 2023 08:42
@MoonE MoonE merged commit deb8022 into openlayers:main Nov 10, 2023
8 checks passed
@MoonE MoonE deleted the regularshape-remove-radius-1 branch November 10, 2023 08:51
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.

2 participants