-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
Adjust Typings for Node Platform #871
Conversation
@KiwiKilian , since you made the original typings PR #766, I was wondering what you think of these changes. Layout wise this looks good to me I think. From the examples in sources code it seems like it makes sense to add these. |
Thanks for your contribution! @etnav 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just some small recommendations from my side.
I struggle to call these functions to validate if it works. For example, when to call setBearing
, setPitch
or setCenter
? My best guess was to call between map.load
and map.render
, but I can't seem to make a difference in the rendered output image. Do you have an example/use case where those functions are used?
@etnav thanks for applying the improvements. Could you provide some usage example?
If it helps, here is a minimal usage example repo: https://github.com/KiwiKilian/maplibre-gl-native-node-example/blob/main/index.ts |
I was trying myself to see if I could get any of these functions to work with https://github.com/acalcutt/maplibre-node-test/blob/main/maplibre-node-test/app.js#L46-L47 , but I had no luck getting them to do anything there either. Not sure I really get the need for some of these functions, wouldn't you have to call render, which has most of these map options (setBearing, setPitch or setCenter) already? |
@KiwiKilian @acalcutt If it helps, I have only used |
I did a lot of searching trying to find any use of these funtions in node, but I only found iOS/Android/Python examples. I'm not really a c++ programmer, but to me it looks like render (center, zoom, bearing pitch) always get set based on RenderOptions. However, when you call SetCenter/SetZoom it doesn't update RenderOptions, instead it tries to set the Camera directrly, which I think later gets overwitten by whats in RenderOptions when render runs. Seems like it would be pretty simple to update RenderOptions when SetCenter/SetZoom gets called, kind of like how those options get updated when you supply options to render EDIT: btw, I don't expect this to be solved in this PR, but I think it explains why these functions seem like they don't work. If these were used on a live view, I think setting camera position makes sense. In this node version I feel like you would always have to render to get anything out... am I wrong on this? If that's the case it seems like SetCenter/SetZoom/etc should set the render option values, so it shows what the user set. |
Question would be, what's the benefit of setting those render options another way? |
I was talking to @tdcosta100 about this yesterday and he found a solution to make those functions work. If I understand correctly, mbgl core has two methods for render. One version takes camera options, and the other takes no options. In the node version, only the render that takes camera options was implemented, so like I found this overwrites the view settings. @tdcosta100 was able to implement the version of render that takes no options, With the second method available, you can call a render with no options, and it just renders whatever view is showing. This allows it to work with the additional functions we couldn't get to work here. Right now you can see the changes he made here, but we should have a PR to discuss it more soon |
Sounds and looks good. Interesting to see what is actually going on in the background. What do you think about the typings changes regarding this PR, do we wait for the new PR or implement only the working parts now? |
I think we should wait for the other PR, so all these functions can work when the typings are released. His changes also add SetSize, which would also be good to add typings for. But this also depends what people think of those changes (they seem pretty minimal to me) |
PR #891 is alive, and if you accept my suggestion: I think all mapped methods can be enriched by their documentation here: https://github.com/maplibre/maplibre-gl-native/blob/main/platform/node/src/node_map.cpp |
I merged #891 . Could Typing for SetSize, and possibly a new one for render without the RenderOptions be added? |
Great, I added |
Before this is merged, can you add a entry to the node changelog, under 5.2.0 Once this is merged I will do another 5.2.0 pre-release so we can test this all together better. |
Adjusted the existing typings and expanded it with some new functions.