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

Fix screenref removal in tesselation #69

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

lennart
Copy link
Contributor

@lennart lennart commented May 19, 2024

This PR fixes removal of screenref vertices (buffer size reduction was previously miscalculated based on size_of PolyVertex!)

It includes an example which triggered this bug (I am OK if the example should not go into main branch, let me know what you think, or how it could be improved)

@kylerchin
Copy link
Contributor

Hello! Thanks for contributing!

Looks like some clippy warnings? If you're not finished with the PR, you can mark it draft. Thank you for contributing to Galileo, this is so important to have a rust map library!
image

@lennart
Copy link
Contributor Author

lennart commented May 22, 2024

@kylerchin I am finished with the PR, but the clippy warnings are not introduced by this code, they are also present on main branch. I guess the clippy and/or rustc stable version have been updated since last time the ci ran...

@lennart
Copy link
Contributor Author

lennart commented May 22, 2024

I guess this is off-topic here, but the latter two clippys may be simply fixed via --fix but the former ones require us to know whether load_image_url and the Renderer trait are supposed to be public, or if they are obsolete...

@kylerchin
Copy link
Contributor

I suppose that's a question for @Maximkaaa on what those functions mean?

@Maximkaaa
Copy link
Owner

Hi, @lennart . Thanks a lot your contribution. I have fixed the clippy warnings with #70, so you can merge in in your brunch to make them go away.

Regarding contents of the PR, that's a nice catch and fix. And especially thank you for the example demonstrating the fix. Having said that, I have some doubts about having the highlight_features example in the main brunch in the current form...

Although it demonstrates the same basic principals of how feature layers work as the feature_layer example, it is definitely simpler and easier to understand for new users. But I tried to make all the examples to also look somewhat nice. And currently the highlight_features example looks like something strange and buggy (even though it is not), especially on higher resolutions.

So, I would be ok with either:

  1. Fix the example to look somewhat presentable, and add it to the galileo/examples/README.md list.
  2. Remove the example altogether. It can even be replaced with a unit test that demonstrates the fix (but I'm ok with skipping that also since there are not many unit tests yet anyway).

BTW, thanks @kylerchin for your active involvement in the project!

@kylerchin
Copy link
Contributor

@lennart just moving this to the top of your inbox!~

@lennart
Copy link
Contributor Author

lennart commented Jun 3, 2024

@Maximkaaa I chose to remove the example, since I also found an issue with image rendering and would rather like to create a prettier example for that. Will open another pr etc. for it.

@Maximkaaa Maximkaaa merged commit d9d4368 into Maximkaaa:main Jun 3, 2024
4 checks passed
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.

3 participants