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

Add SDFGeometryGenerator addon. #26837

Merged
merged 22 commits into from
Oct 11, 2023
Merged

Add SDFGeometryGenerator addon. #26837

merged 22 commits into from
Oct 11, 2023

Conversation

santi-grau
Copy link
Contributor

New Addon

Description

Creates a THREE.BufferGeometry from an SDF.
Added example and documentation

examples/jsm/geometries/SDFGeometry.js Fixed Show fixed Hide fixed
examples/webgl_geometry_sdf.html Fixed Show fixed Hide fixed
examples/webgl_geometry_sdf.html Fixed Show fixed Hide fixed
examples/webgl_geometry_sdf.html Fixed Show fixed Hide fixed
~ Code scanning checks
~ Update class name to SDFGeometryGenerator
~ Change class design
~ Remove confirm dialog
~ Standardize example structure
~ Remove shader editor
- Remove extends Buffergeometry
- Replace logs by error
- Added option for more res if device supports 32k texture
- Remove unused properties
@gkjohnson
Copy link
Collaborator

gkjohnson commented Sep 27, 2023

Just a couple thoughts while passing through -

  • It looks like this is implementing SDF to geometry functionality with "Surface Nets" which is basically a Marching Cubes alternative. It would be nice to align the API with the current MarchingCubes object implementation since in theory surface nets should be a drop in replacement if I'm understanding right. Though the current MarchingCubes class is structured a bit oddly extending from Mesh so maybe this can be figured out later.
  • It looks like a lot of the code is copy-pasted from this library file here in another repo. It might make most sense to drop that file (modified to support exports) in the jsm/libs directory with the other library files along with a date of copy / version number so it's easier to keep track of.

Overall surface nets look cool, though - I hadn't heard of them before!

@santi-grau
Copy link
Contributor Author

santi-grau commented Sep 27, 2023

@gkjohnson thanks for your input.
Regarding moving this to use Marching Cubes, I'm afraid is a bit beyond my skills :( that's why I used Mikola's isosurface.
Are there any docs on how a file at libs should be included? Or just remove from SDFGeometryGenerator.js and copy at jsm/libs?

ty!

@gkjohnson
Copy link
Collaborator

Regarding moving this to use Marching Cubes

I didn't intend to suggest using MarchingCubes - just that the APIs between the two classes should (some day) be the same since they aim to solve the same problem. I think that can be skipped in this PR.

Or just remove from SDFGeometryGenerator.js and copy at jsm/libs?

This one. Ideally the code is formatted the same as the original source so it's easy to diff and update if needed. It will also need to be adjusted to support js exports.

@santi-grau
Copy link
Contributor Author

@gkjohnson moved the surfaceNets script onto jsm/libs

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 29, 2023

@santi-grau I've went over the code to improve the code style and to simplify the example at a few places. The most important thing for me was to avoid custom material shaders in webgl_geometry_sdf. So the meshes are now rendered with built-in materials. This will make it easier to port the example to WebGPU at a later point.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 30, 2023

@santi-grau The PR looks in general good to me. The only thing that might be a problem is that the example relies on custom GLSL code. This dependency will make it harder to migrate the demo to WebGPURenderer at some point since the GLSL needs to be migrated to TSL to work with both WebGPU and WebGL.

Would you be okay to add just the Mandelbrot shader and remove the two others? The head and Suzanne shader are so large that rewriting the code to something else will be quite time-consuming.

@santi-grau
Copy link
Contributor Author

Sure, will update!

Remove large glsl examples
Take screenshot
@santi-grau
Copy link
Contributor Author

Updated the example removing those two shaders.

Mugen87
Mugen87 previously approved these changes Oct 5, 2023
@Mugen87 Mugen87 changed the title Sdf geometry addon Add SDFGeometryGenerator addon. Oct 5, 2023
@Mugen87 Mugen87 added this to the r158 milestone Oct 9, 2023
@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

@santi-grau While improving the example, I have noticed SDFGeometryGenerator makes no use of dispose() methods which means the component creates memory leaks. I have refactored the code a bit to fix this issue. You can easily see the effect when inspecting the renderer.info debug object after generating some geometries.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

I've realized the rendering performance of computeSDF() could be noticeably improved by using instanced rendering because the geometry (the plane) is the same for each draw call and only the tileNum uniform is different. If tileNum and the position of the plane would be instanced attributes, computeSDF() would only require a single draw call, no matter how many tiles are configured.

Would you be willing to give this implementation a try?

@Mugen87 Mugen87 dismissed their stale review October 10, 2023 08:50

Further changes are requried.

Simplify usage of `setIndex()`.
@santi-grau
Copy link
Contributor Author

@santi-grau While improving the example, I have noticed SDFGeometryGenerator makes no use of dispose() methods which means the component creates memory leaks. I have refactored the code a bit to fix this issue. You can easily see the effect when inspecting the renderer.info debug object after generating some geometries.

Didn't know we had to manually dispose. Thanks for adding!

@santi-grau
Copy link
Contributor Author

santi-grau commented Oct 10, 2023

I've realized the rendering performance of computeSDF() could be noticeably improved by using instanced rendering because the geometry (the plane) is the same for each draw call and only the tileNum uniform is different. If tileNum and the position of the plane would be instanced attributes, computeSDF() would only require a single draw call, no matter how many tiles are configured.

Would you be willing to give this implementation a try?

The reason why I'm using 'tile rendering' is to prevent crashes in case the SDF function gets too complex ( e.g. fbm ). This way, no matter what, we are just computing a specific tile size. If you still think that instance rendering will still be safe and more efficient I can try to implement it.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 10, 2023

The reason why I'm using 'tile rendering' is to prevent crashes in case the SDF function gets too complex

I see. In this case, I think it's okay to start with the current implemented approach. If users report performance issues, we can revisit this topic (and maybe think about different rendering modes).

@Mugen87 Mugen87 merged commit eb073dd into mrdoob:dev Oct 11, 2023
17 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