-
Notifications
You must be signed in to change notification settings - Fork 327
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
Performance Enhancements for Web Viewer #1891
base: main
Are you sure you want to change the base?
Conversation
// Note: that data blobs and embedded data textures are not cached as they are transient data. | ||
let checkCache = true; | ||
let texturePath = searchPath + IMAGE_PATH_SEPARATOR + value; | ||
if (value.startsWith('blob:')) |
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.
Blobs, embedded data and http references are not relative to a search path. This fixes it so users can use these texture URI specifications in their files.
checkCache = false; | ||
console.log('Load data URL:', texturePath); | ||
} | ||
const cachedTexture = checkCache && THREE.Cache.get(texturePath); |
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.
Do proper cache checking.
@@ -179,8 +186,19 @@ function animate() | |||
viewer.getScene().setUpdateTransforms(); | |||
} | |||
|
|||
renderer.render(viewer.getScene().getScene(), viewer.getScene().getCamera()); | |||
viewer.getScene().updateTransforms(); | |||
// Only re-render when an update request was made |
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.
Re-render gated by requirement to refresh. All the schedulUpdate()
calls triggers the requirment.
@@ -622,7 +627,12 @@ export class Material | |||
|
|||
// Load material | |||
if (mtlxMaterial) | |||
await mx.readFromXmlString(doc, mtlxMaterial, searchPath); | |||
try { |
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.
Small fix. Would "crash" sometimes on invalid files.
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.
I really like all of the small fixes and improvements in this change, but I'm less convinced by the caching scheme, which runs counter to the rendering approach I see in other performance-focused web applications. Additionally, the caching scheme has the side effect of making viewer
an input to toThreeUniform
, which doesn't seem ideal.
What would you think about preserving all of the small fixes and improvements in this change, but omitting the caching scheme for now, and we can consider this idea separately in the future?
Sounds like a good separation. Maybe let's table looking at why "render" has so much overhead. It could be draw time, it could be uniform bind time or state setup time or something else. OpenPBR is the slowest shader but std surface is also good for testing. |
Performance Enhancements
Implementation Changes
Note: There is no refresh queue management (e.g. compressing the number of requests) so that all camera, property edits, turntable updates are queued. This will never be worse than what occurs before this change and is thus as "safe" as before (no refreshes will be "list". Re-render is gated by how often ThreeJS triggers a refresh/render.
@jstone-lucasfilm : This makes OpenPBR not max-out the GPU on my laptop and should help a fair bit with scenes with more textures such as the chess set as the texture cache is used. Will also help drag-drop.
Example