-
Notifications
You must be signed in to change notification settings - Fork 394
Add spark.js support to three.js 3DTilesRenderer #1497
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
base: master
Are you sure you want to change the base?
Changes from all commits
1de40ff
601cd19
08c227f
b96fd14
73f9481
9a5171e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,7 @@ | |
| "@babylonjs/core": "^8.47.2", | ||
| "@babylonjs/loaders": "^8.47.2", | ||
| "@eslint/js": "^9.0.0", | ||
| "@ludicon/spark.js": "^0.1.3", | ||
| "@react-three/drei": "^10.0.0", | ||
| "@react-three/fiber": "^9.0.0", | ||
| "@types/node": "^24.3.0", | ||
|
|
@@ -108,7 +109,7 @@ | |
| "leva": "^0.10.0", | ||
| "lil-gui": "^0.21.0", | ||
| "postprocessing": "^6.36.4", | ||
| "three": "^0.170.0", | ||
| "three": "^0.182.0", | ||
| "typescript": "^5.6.0", | ||
| "typescript-eslint": "^8.48.1", | ||
| "vite": "^6.2.2", | ||
|
|
@@ -120,7 +121,7 @@ | |
| "@react-three/fiber": "^8.17.9 || ^9.0.0", | ||
| "react": "^18.3.1 || ^19.0.0", | ||
| "react-dom": "^18.3.1 || ^19.0.0", | ||
| "three": ">=0.167.0" | ||
| "three": ">=0.182.0" | ||
|
Comment on lines
-123
to
+124
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't change the peer dependency requirements for the project
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll need to double check whether the required three.js features are all present in that older version, and if they don't gracefully handle the error. It may just work, since most of the three.js changes were in the WebGPU backend, and to add better support for normal maps, which these demos don't need.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spark.js is not a dependency or requirement of this project so the peer dependency should not change - users can use and install 3d-tiles-renderer with r167+ just fine. Spark.js will need to specify it's own three.js dependency limit so if users install Spark that peer dependency version will need to be respected. |
||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@react-three/fiber": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { estimateBytesUsed as _estimateBytesUsed } from 'three/examples/jsm/utils/BufferGeometryUtils.js'; | ||
| import { TextureUtils } from 'three'; | ||
| import { TextureUtils, ExternalTexture } from 'three'; | ||
|
|
||
| export function getTextureByteLength( tex ) { | ||
|
|
||
|
|
@@ -9,6 +9,13 @@ export function getTextureByteLength( tex ) { | |
|
|
||
| } | ||
|
|
||
| // IC: The code below assumes tex was created from an ImageBitmap | ||
|
gkjohnson marked this conversation as resolved.
|
||
| if ( tex instanceof ExternalTexture && tex.userData?.byteLength ) { | ||
|
|
||
| return tex.userData.byteLength; | ||
|
|
||
| } | ||
|
Comment on lines
+12
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if an "ExternalTexture" is provided without userData.byteLength? It looks like "image" will be "null" on the Texture, then and the follow code will crash, right? Assuming we can't get the actual texture size from the ExternalTexture handle we should figure out a reasonable default behavior when byteLength isn't provided.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code will crash with ExternalTextures generated by other tools. It also won't compute the size of CompressedTextures correctly, as the generateMipmaps parameter is forced to false, but textures may still have mipmaps. I'd be happy to robustify this code. I was aiming for minimal changes here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making it more robust would be great. I expect it's not possible to determine the actual in-memory size of one of these textures from just the WebGLTexture (or WebGPU) handle itself, right? At least not without the gl contenxt. In lieu of any other information being available for calculating the size I was thinking we could check for a custom field (as you are here) and otherwise fallback to a fixed memory size of a 64x64 texture or something just so the value isn't "0" in the cache.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll propose that change in a separate PR. Here I just wanted to keep it simple to demonstrate that the changes required for integration are minimal.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be great. A PR to improve handling of just ExternalTextures is something we could merge immediately. A couple other things that come to mind that need to be handled:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hm, my original intention for THREE.ExternalTexture had been that if the user (or a library the user delegates to) creates a GPU resource, they must also manage the GPU resource throughout its lifecycle. From that perspective:
If the user is relying on Spark to create these ExternalTextures, it becomes Spark's responsibility to either manage the resource lifecycle, or communicate that responsibility to the user. If the user has created an ExternalTexture manually or with some other dependency, it would be the user and/or dependency's responsibility to do the same. The proposal Ludicon/spark.js#31 is consistent, I think, in defining a clear contract for the lifecycle of the ExternalTexture and its GPU resource. Arguably, GLTFLoader is not doing so good of a job there, by constructing ImageBitmap textures in unspecified conditions, and leaving it to the user to dispose those resources manually... which I'd guess is often overlooked. I don't have an easy fix for that, but it's not a pattern I'd want to promote. I think it's not practical for an ExternalTexture to store enough metadata for its GPU texture to be recreated after disposal, would you agree? The original image should no longer exist in memory. Assuming so... maybe a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @donmccurdy I guess fundamentally I'm confused as to why one would advocate for his behavior in the spark library but not with ImageBitmap in GLTFLoader - it's just as possible for GLTFLoader to register the same kind of disposal event but I think it's known this causes massive usability issues, which is why it is the way it is.
To be clear I completely agree that symmetry in the life cycle of the texture is both a nicer and safer pattern. The problem is the practical issues it causes. The suggested pattern betrays the expectations by every other Texture instance in the project. Off the top of my head my own real-world use cases this would have broken if it behaved this way:
All of these very reasonable use cases would wind up breaking if textures were managed in the way that is being suggested.
This isn't how three.js' API presents itself though. Perhaps it's just becoming clear that three.js' API surface was not designed for these kinds of use cases if even just because the original source image textures used did not have these issues. Why should "dispose" behave inconsistently for image bitmap or external texture? Perhaps it's just the way I'm thinking about but as I mentioned before, "dispose" has never meant "dispose content forever" in three.js. Perhaps there's are argument to be made for adding a "disposeForever" function to three.js textures to accommodate these types of use cases.
Of course agree with this. But I also think that I should also be able to call "dispose" and expect consistent behavior. These classes are supposed to be reliable abstractions with consistent, usable behavior so I'm not in agreement that the "dispose" function should suddenly be overloaded to completely discard resources and make a texture instance unusable, which again doesn't happen anywhere else.
My preference is to not add library-specific code throughout the project to accommodate every special case someone may come to the project with. -- Either way if this is the preference for how to manage the disposal of these textures that's fine. I only intended to point out what will be ergonomic issues down the line (and within this project).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The more I learn about how ExternalTextures work, the more confused I am. The behavior of the WebGL and WebGPU backends are entirely different. In WebGPU, it appears that the behavior depends on whether the texture has been used for rendering or not. During rendering, three calls This is quite confusing. If you call dispose() prior to rendering, nothing happens, if you call it after, the texture is destroyed. If you have multiple textures referencing the same GPUTexture, but some have been used for rendering and others haven't, then the behavior is completely broken. The WebGL backend doesn't appear to behave like that, and it never takes ownership of the textures objects. This is at least more consistent. I'm working on a solution that involves the creation of a map to manage the lifetime of spark external textures. Instead of using THREE.ExternalTexture to create a texture, you use a derived class that overrides the copy constructor and the dispose method and tracks the gl object associated with spark and the number of textures that reference it, so that it can dispose the texture object when no texture references it anymore. I'll post an update once I have something solid.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, here's what I have: Usage is as follows: in the case of the 3DTilesRenderer no changes are necessary. The GLTF loader plugin takes care of creating the external texturs using the SparkThreeExternalTexture class. It works as expected in my examples, but it needs more testing. Let me know what you think of this approach.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This difference in behavior between WebGLRenderer and WebGPURenderer may not be intended or desired. WebGPURenderer is still very much in development and there are still quite a few oversights like this throughout.
This doesn't completely address everything but I think this is fundamentally something that three.js needs to promote a more consistent pattern for if it's going to become more commonly used. I think we can wait for more problems to arise before trying to address this further. |
||
|
|
||
| const { format, type, image } = tex; | ||
| const { width, height } = image; | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.