nmwsharp / polyscope

A C++ & Python viewer for 3D data like meshes and point clouds
https://polyscope.run
MIT License
1.81k stars 198 forks source link

Added `SurfaceTextureQuantity` in order to support custom textures #197

Closed amitdiamant closed 11 months ago

amitdiamant commented 1 year ago

Added custom texture support for surface meshes. This PR is based on the suggestions given here: https://github.com/nmwsharp/polyscope/issues/111#issuecomment-850448308

Usage example:

polyscope::Texture texture{};
int comp;
texture.data = stbi_load("/path/to/texture.png", &texture.width, &texture.height, &comp, STBI_rgb_alpha);
polyscope::getSurfaceMesh(surfaceName)->addTextureQuantity("texture", vertParam, texture);
Screenshot 2023-02-22 at 18 22 44

Added some additional APIs for changing the UVs and texture of the quantity after initialization: SurfaceTextureQuantity::setTexture, SurfaceTextureQuantity::setUVs.

(Also fixed some whitespace issues. for easy reviewing, use &w=1 in the URL)

nmwsharp commented 1 year ago

Great! This was on my radar for the near future already, so it would be awesome to merge this in.

One design question: it looks like this implementation is configured to pass both UVs and the texture itself into the SurfaceTextureQuantity. However, it's likely that many uses would apply many different textures all with the same UV map. And also, we already have the ParameterizationQuantity classes with some nice features to visualize UVs themselves. What if we configured the SurfaceTexture class to instead just take the texture as input, along with a name/pointer to an existing SurfaceParameterizationQuantity which it pulls the UVs from? We could also create a helper function that creates both simultaneously if we want.

amitdiamant commented 1 year ago

@nmwsharp

What if we configured the SurfaceTexture class to instead just take the texture as input, along with a name/pointer to an existing SurfaceParameterizationQuantity which it pulls the UVs from? We could also create a helper function that creates both simultaneously if we want.

OK, just to make sure - you still mean that it SurfaceTexture will be a quantity of it own (SurfaceTextureQuantity), but just that it will reuse the UVs from an existing SurfaceParameterizationQuantity, right?

What do you think of having set of two constructors / setters for this? One where the client explicitly provides the UVs (as in the initial commit that I made), and another with a pointer to SurfaceParameterizationQuantity that it will take the UVs from there?

In other topic - I noticed that the build fails (also in other existing PRs) due to unused variables. I think it should be fixed by removing some compiler flags, such as Wunused-but-set-variable (or actually deleting the unused vars).

nmwsharp commented 1 year ago

OK, just to make sure - you still mean that it SurfaceTexture will be a quantity of it own (SurfaceTextureQuantity), but just that it will reuse the UVs from an existing SurfaceParameterizationQuantity, right?

What do you think of having set of two constructors / setters for this? One where the client explicitly provides the UVs (as in the initial commit that I made), and another with a pointer to SurfaceParameterizationQuantity that it will take the UVs from there?

Yes and yes. These both sounds good to me.

I noticed that the build fails (also in other existing PRs) due to unused variables

Indeed. We should really just remove -werror from the build scripts so that silly new warnings like this don't break builds.

nmwsharp commented 1 year ago

Removed the Werror flag in c772fd9. Rebasing should unbreak the build (still waiting for CI to run to confirm).

amitdiamant commented 1 year ago

@nmwsharp Updated according to your suggestion - let me know if that's ok :)

JeffreyLayton commented 11 months ago

Is there any update to this?

nmwsharp commented 11 months ago

Yes! Texture support has been recently merged. The approach is combination of this PR & a similar PR by Mark Gillespie. Both color and scalar textures are supported, and they pull their UV coordinates from a separate ParameterizationQuantity.

It's not yet documented on the docs website yet. I'm trying to get those docs written up ASAP for this and some other changes to cut a new version, hopefully in the next week or two 🤞.

In the meantime: