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.

Sign up for GitHub

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

Jump to bottom

Fixes / improvements for RegularShape #12467

Merged
merged 10 commits into from
Jul 11, 2021
Merged

Fixes / improvements for RegularShape #12467

merged 10 commits into from
Jul 11, 2021

Conversation

MoonE
Copy link
Contributor

@MoonE MoonE commented Jul 4, 2021 •
edited
Loading

Fixes #12461

  • Lazily initialize the canvases, and do not create some that were never needed.
  • Canvas size was too small if the miter length is larger than the stroke width
  • Canvas size was too large by one stroke width for round line join
  • Reduce canvas size more for bevel line join
  • bevel/miter line join missing on last/first point because path was not closed before stroking
  • Implement high pixel ratio regular shape for immediate renderer
  • It should always double the number of points when radius2 is provided.
  • Don't require radius1 to be greater than radius2

@MoonE MoonE force-pushed the fixes-regular-shape branch 4 times, most recently from eca9582 to d28c5c3 Compare July 5, 2021 23:03
@ahocevar ahocevar mentioned this pull request Jul 7, 2021
Closed
@ahocevar
Copy link
Member

ahocevar commented Jul 7, 2021

Great work @MoonE! Let me know if you want me to look into anything specifically, or if you need help finishing this pull request.

MoonE
MoonE commented Jul 8, 2021
View reviewed changes
src/ol/style/RegularShape.js Outdated Show resolved Hide resolved
@MoonE MoonE force-pushed the fixes-regular-shape branch 3 times, most recently from 0808709 to dbe284b Compare July 11, 2021 01:27
@MoonE MoonE changed the title Fixes regular shape Fixes / improvements for RegularShape Jul 11, 2021
@MoonE MoonE marked this pull request as ready for review July 11, 2021 01:36
@MoonE
Copy link
Contributor Author

MoonE commented Jul 11, 2021

Setting the correct canvas size works, except with very pointy star, then it is still beveled. Maybe there is some hardcoded limit?
Though I don't think it is a real problem.

@MoonE MoonE force-pushed the fixes-regular-shape branch 2 times, most recently from 664af5e to 7847d39 Compare July 11, 2021 03:08
ahocevar
ahocevar approved these changes Jul 11, 2021
View reviewed changes
Copy link
Member

@ahocevar ahocevar 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.

This looks excellent, @MoonE! Please check the failing rendering tests - if it's just pixel shifts caused by the fix, you can replace the reference images with those provided in the rendering test artifacts. Let me know if you need help with that. Also, please remove the test example. Then you can merge. Thanks!

@MoonE
No unnecessary canvas with transparent fill
82265ef 
With pixelRatio != 1 and a transparent fill a unnecessary image was created.
@MoonE
Cleanup drawing of RegularShape
5716c61 
Fixes missing miter/bevel line join because path was not closed before
stroking.
Reduce canvas context calls, skip beginPath, and instead of drawing the
last point use closePath
@MoonE
getImage is always called with valid pixelRatio
28c40fe
@MoonE
Consistent behaviour if two radii are provided
f21513a 
Always double the points event when two radii of same size are provided.
@MoonE
Fix some RegularShape bugs related to canvsa size
87f2159 
- remove jitter when using RegularShape size in animation, size should
always be an integer
- canvas may have been too small for miter line join
- canvas was at least one stroke width too large for round line join
- reduce canvas size even more for bevel line join

Canvas now precisely fits the shape including stroke, The angle of the
shape is ignored for the calculation.
@MoonE
High resolution image style for immediate renderer
f1ecc87
@MoonE
Lazily create Regular shape canvases
53cf5bb 
Creating the cached canvases is expensive and they may never be used.
E. g. the hit detect canvas is not needed with the Immediate renderer
and if a feature is never in the viewport the image is not needed either.
@MoonE
cache render options object
bee28d6
@MoonE
Remove redundant size properties / getter
ad37baf
@ahocevar ahocevar self-requested a review July 11, 2021 11:29
ahocevar
ahocevar requested changes Jul 11, 2021
View reviewed changes
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

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

There is a problem when rendering circles in vector tiles. A square is drawn around the circle. See this enlarged image from the failing layer-vectortile-simple rendering test:

Untitled

@MoonE
Copy link
Contributor Author

MoonE commented Jul 11, 2021

That is not the latest image. This was for testing, but it is already removed when I remove the example

@ahocevar
Copy link
Member

Ah cool. While I'm at checking the rendering tests, I could add a commit with the updated reference images. Do you want me to do that, or are you already at it?

@ahocevar ahocevar self-requested a review July 11, 2021 12:21
ahocevar
ahocevar approved these changes Jul 11, 2021
View reviewed changes
Copy link
Member

@ahocevar ahocevar 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.

I just pushed that commit, @MoonE. Now rendering tests should pass. If they do, please merge.

@MoonE
Copy link
Contributor Author

MoonE commented Jul 11, 2021

Thanks you, @ahocevar

@MoonE MoonE merged commit bb0ea08 into openlayers:main Jul 11, 2021
@MoonE MoonE deleted the fixes-regular-shape branch July 11, 2021 13:47
mike-000 added a commit to mike-000/openlayers that referenced this pull request Aug 8, 2022
@mike-000
Do not apply openlayers#12467 change to Icons
0785d46
mike-000 added a commit to mike-000/openlayers that referenced this pull request Aug 8, 2022
@mike-000
Do not apply openlayers#12467 change to Icons
4953f63 
Use getPixelRatio() as in ImageBuilder
ahocevar added a commit that referenced this pull request Aug 12, 2022
@ahocevar
Merge pull request #13955 from mike-000/immediate-icons
3e707d4 
Do not apply #12467 change to Icons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahocevar ahocevar ahocevar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Circle's animation jitter
2 participants
@MoonE @ahocevar