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

Update Magnum submodules with support for querying MeshData size from Python #1878

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Sep 29, 2022

Motivation and Context

I was actually false-advertising on the call that it's possible to query trade.MeshData.index_data and trade.MeshData.vertex_data size from Python. So I added those.

For images, trade.ImageData2D.data was a thing before already. Those are byte array views, so len() is the byte count.

Also contains all other submodule updates that are a part of #1874 as well. Nothing controversial or attention-worthy there, since all those changes are mostly just related to glTF export that only the batch renderer uses.

How Has This Been Tested

My CIs pass.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 29, 2022
@Skylion007
Copy link
Contributor

Nit: on the magnum-bindings. You cast from handle to C++ back to handle with py::cast(self). You could just have self start as py::handle cast to C++ and save a conversion.

@mosra
Copy link
Collaborator Author

mosra commented Sep 29, 2022

Hm, but there I need to access self.indexData() for example. So I would need to (manually) cast to C++ anyway, no? Not sure what you mean exactly.

@Skylion007
Copy link
Contributor

Hm, but there I need to access self.indexData() for example. So I would need to (manually) cast to C++ anyway, no? Not sure what you mean exactly.

Ah, I guess in this case it doesn't make quite as much sense. It's very useful for when you are using only python APIs. Also, it would save you a python->C++->python roundtrip, but probably not worth the added complexity here.

@mosra
Copy link
Collaborator Author

mosra commented Sep 30, 2022

Yeah, here it's not that much of a perf bottleneck, so I prefer to have it done the least error-prone way possible.

Anyway, thanks for the hints, as always. Much appreciated :)

@mosra mosra merged commit 4cc69db into main Sep 30, 2022
@mosra mosra deleted the update-magnum17 branch September 30, 2022 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants