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

Editor: Update realistic viewport packages #27780

Merged
merged 5 commits into from
Feb 20, 2024

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Feb 20, 2024

Related issue: -

Description

  • Bump three-mesh-bvh to v0.7.3
    • Fixes for inverted geometry
    • Fixes for handling scenes with no geometry
  • Bump three-gpu-pathtracer to v0.0.19
    • More resiliently handle non-float data textures for environment map
    • Handle black environment maps
    • Better sampling patterns
    • more
  • Editor changes
    • Use a black environment map if one isn't provided like the main view
    • Don't render the path traced view until 1 sample is complete
    • Remove dummy data if no geometry is present
    • Use a 4x4 texture for the env map since there seems to be a small issue in the sampling logic with a 1x1

Spot lights working:

image

cc @mrdoob it looks like init gets called every time the scene is modified. Is this correct? If so that means a lot of textures and materials are getting recreated with out ever being disposed.

@gkjohnson gkjohnson added this to the r162 milestone Feb 20, 2024
@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

cc @mrdoob it looks like init gets called every time the scene is modified. Is this correct? If so that means a lot of textures and materials are getting recreated with out ever being disposed.

It is not correct but it was the quickest way to get things working last month 😇

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

I'll merge and try to remove the init tomorrow.

@mrdoob mrdoob merged commit 54127d1 into mrdoob:dev Feb 20, 2024
11 checks passed
@gkjohnson gkjohnson deleted the update-realistic-viewport branch February 20, 2024 14:30
@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

I guess now that we're on the topic...

If the scene graph changes, or a geometry changes, or a material changes... What should I update?

@gkjohnson
Copy link
Collaborator Author

I can take a look and adjust things once "init" is changed to actually just init once (simplifying the boilerplate for a renderer is on my list). Mostly it's the textures that are being generated with "buildColorTexture" every time "init" is called right now. Everything else is guarded behind "pathtracer === null" right now which is good.

@mrdoob
Copy link
Owner

mrdoob commented Feb 20, 2024

Mostly it's the textures that are being generated with "buildColorTexture" every time "init" is called right now. Everything else is guarded behind "pathtracer === null" right now which is good.

Ah! These doesn't seem that bad then...

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Feb 21, 2024

Yeah the textures aren't so bad. I'm playing around a bit more, though, and the other issue is that right now the BVH and merged geometry are rebuilt every single change. This isn't a memory issue but it can be very slow if the scene / geometry is complex.

If objects are moved or adjusted then we can refit the BVH but I don't know how easy it is to determine when to do that. At the least the model shouldn't be rebuilt if just the camera is moved or user clicks. I'll have to think about it a bit more. Longer term it would be good to use a worker to build the BVH, as well.

@mrdoob
Copy link
Owner

mrdoob commented Feb 21, 2024

At the least the model shouldn't be rebuilt if just the camera is moved or user clicks.

Yes, I can take care of these.

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2024

Done! 5838c86

Seems like we also need to call init to update the background and environment.
Is that expected?

@gkjohnson
Copy link
Collaborator Author

Seems like we also need to call init to update the background and environment.
Is that expected?

I can take a look later. What exactly is happening?

@mrdoob
Copy link
Owner

mrdoob commented Feb 27, 2024

Just seems like there is no way to change the background/environment settings without calling init.

@gkjohnson
Copy link
Collaborator Author

Oh I see - as it's implemented now, yes. From the commit I see how events are propagated, now, so it should be possible to provide a more granular message to the pathtracer telling it what should be updated. At the moment init is updating everything including the geometry, bvh, environment map, etc.

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.

2 participants