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

Support for hit detection in styles with custom rendering #12646

Merged

Conversation

ashchurova
Copy link

@ashchurova ashchurova commented Aug 20, 2021

This fix is for Issue 8136. As per @ahocevar request defined new optional property for ol.style.Style type called hitDetectionRenderer and it is used if specified. If it is not specified logic in CanvasBuilder.drawCustom falls back to passed renderer. This is my first time contributing so I am new to the process. @ahocevar you did mention in your comments that you wanted use hitDetectionRenderer when no custom renderer is specified. Still researching how to do that. Because that would mean recoupling drawing from hit detection instructions.

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.

Good work! As far as I can tell, this is exactly what I had in mind. Maybe add a test and an example, then this should be good.

@ashchurova
Copy link
Author

@ahocevar OK :) Sorry for the delayed response, had a long labor day weekend. So I will add tests and and example to your samples library. I need to think about what would be a good case to have different renderer for hit detection.

@ashchurova
Copy link
Author

@ahocevar Hi Andreas, I am having trouble starting serve-examples while working on the example. When I run npm run serve-examples I get an error:

[webpack-cli] Failed to load 'C:\Dev\openlayers_my_fork\examples\webpack\config.mjs' config
[webpack-cli] Error: Not supported

Any suggestion on how to fix this one?

@ashchurova ashchurova marked this pull request as ready for review September 13, 2021 20:44
@ashchurova
Copy link
Author

@ahocevar Hi Andrea, added a sample for custom hit renderer, I kind of based it on the existing custom renderer sample for circle and showed that only label is used for hit detection. Let me know what you think. Thanks.

Copy link
Contributor

@MoonE MoonE 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 working on this neat feature, @ashchurova.

examples/custom-hit-detection-renderer.js Outdated Show resolved Hide resolved
examples/custom-hit-detection-renderer.js Outdated Show resolved Hide resolved
examples/custom-hit-detection-renderer.js Outdated Show resolved Hide resolved
examples/custom-hit-detection-renderer.js Show resolved Hide resolved
@ashchurova
Copy link
Author

@ahocevar Hi Andreas, no sure why these unit tests are failing on the server. All unit tests pass on my machine. Could it be that there is a race condition some sort? Thank you.

@ashchurova
Copy link
Author

ashchurova commented Sep 15, 2021

@ahocevar Hi Andreas, no sure why these unit tests are failing on the server. All unit tests pass on my machine. Could it be that there is a race condition some sort? Thank you.

Now Browser tests ran passed, interesting.. @MoonE I have addressed all your requested changes.

@ashchurova
Copy link
Author

@ahocevar any update on when this PR will be merged or any other feedback? Thanks.

@ahocevar
Copy link
Member

Hey @ashchurova, sorry this is taking so long. Unfortunately I have been too busy with my day job and FOSS4G preparation. I'll be looking into it as soon as possible.

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.

Excellent work, @ashchurova! Thanks a lot for this contribution, and thanks for adding an example that shows custom style renderers in action.

@ahocevar
Copy link
Member

@MoonE, I think all your comments were properly addressed, but please double-check and approve when you're happy too.

Copy link
Contributor

@MoonE MoonE left a comment

Choose a reason for hiding this comment

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

The requested changes are good, thanks @ashchurova.

@MoonE MoonE merged commit 9e37182 into openlayers:main Sep 29, 2021
@ashchurova
Copy link
Author

@ahocevar @MoonE Yeay!! When do you plan to publish this change? Thank you.

@ahocevar
Copy link
Member

ahocevar commented Sep 30, 2021

@ashchurova It is already published on the dev tag. npm install ol@dev will pull that for you. It will of course be in the next release. We published v6.8 a week ago, and we publish releases approximately every 2 months.

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.

4 participants