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

[Qt for WebAssembly; opengl-2 branch] Fix http_file_source #2689

Open
wants to merge 1 commit into
base: opengl-2
Choose a base branch
from

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Aug 5, 2024

The manual changes required for qt webgl1 compilation are here fixed with compile conditionals:

https://github.com/birkskyum/maplibre-native-wasm/tree/main/qt-opengl2#changes-needed-to-the-code

(i'll make sure to clean up this branch - made it in main repo instead of fork by accident)

@birkskyum birkskyum requested a review from ntadej August 5, 2024 22:50
@birkskyum birkskyum changed the title Fix qt wasm webgl1 compilation [Qt for WebAssembly; opengl-2 branch] Fix http_file_source Aug 6, 2024
@ntadej
Copy link
Collaborator

ntadej commented Aug 6, 2024

This is still causing a leak in WASM build. As this is not urgent, let me try to find a proper fix.

Copy link
Collaborator

@ntadej ntadej left a comment

Choose a reason for hiding this comment

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

Let me also do a review.

@birkskyum
Copy link
Member Author

birkskyum commented Aug 13, 2024

This is still causing a leak in WASM build.

This actually closes a leak in WASM build by allowing the browser to abort tile requests. It's easy to verify, because without this fix the app stalls and crashes shortly after boot due to a lack of cleanup.

@ntadej Can we maybe merge this fix, so that things work out-of-the-box, and then at your convenience you can look into improving on it further if the opengl-2 branch still is relevant at the time?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 9, 2024

@ntadej , is it okay with you if we merge the fix, so that the code can at least run without the manual edits, and then investigate the persisting memory leak later on?

@louwers
Copy link
Collaborator

louwers commented Nov 9, 2024

@birkskyum Your suggestion sounds reasonable to me. I think @ntadej was afk last month, let's give him a chance to respond this weekend and merge on Monday otherwise.

@ntadej
Copy link
Collaborator

ntadej commented Nov 10, 2024

Sorry, I did not see that you condition now with WASM. I think it's fine for now until I have time to investigate closer.

Note that I am a bit disappointed in MapLibre memory usage so I plan to investigate eventually if there are not more leaks.

@ntadej
Copy link
Collaborator

ntadej commented Nov 10, 2024

Before we merge, is this already in main? If not, in principle all changes go there first to avoid forgetting something.

@louwers louwers changed the base branch from opengl-2 to main November 10, 2024 19:48
@louwers louwers changed the base branch from main to opengl-2 November 10, 2024 19:48
@louwers
Copy link
Collaborator

louwers commented Nov 10, 2024

No it's not @birkskyum Could you make a new PR that targets main?

@josxha
Copy link
Contributor

josxha commented Nov 10, 2024

@louwers
Copy link
Collaborator

louwers commented Nov 10, 2024

@josxha I tried, but then there's a bunch of unrelated changes in the PR...

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.

5 participants