-
Notifications
You must be signed in to change notification settings - Fork 105
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
Added basic EXT_mesh_features samples #40
Conversation
@javagl tried out your examples. I can reproduce the problems for the first 4. For the 5th one, it loads fine, though it looks like the string property is misalligned, see here how the property value is "Rai" when I click the bottom left square, and if I click the bottom right one I get the "n" for "Rain" |
One more sample idea that could be considered for a follow up PR - two primitives with features that point to the same property table. |
There was a wrong index for the property tables that caused the error that I originally considered to be some endianness issue. Sorry about that, it should be fixed with the last commit. |
Just noticed a typo in the example data...
That should have been 2.1. It's the details... (will update that at some point...) |
In response to #40 (comment) , I just added an example that contains two primitives that refer to the same property table. I'm still in the process of trying to find "the best way" to create these samples, but I'll consider and try out other possible (more "complex") examples, and add them here when ready. |
The binary payload for the properties now contains the right values for the vector (a detail). The MultipleFeatureIdsAndProperties example now contains properly encoded strings.
The The reason for the wrong alignment was that I had created the buffer views as |
I have added a first, basic example for Feature ID Textures. It might look familiar to some of you. I'll have to look into the details on how the sandcastle has to be updated for this (i.e. to actually look up the feature ID properly - right now, the tooltip always shows the info for Feature ID 0 (Wall)). @ptrgags Even though texture filtering is set to 9728 (NEAREST), there seems to be some "blurring". The textures right now have a size of 10x10 - could it be that they are resized to a power-of-2 (for some WebGL 1 compatibility or so)? (For comparison, the screenshot shows the three.js viewer result). And I'll attach the textures here, just for the giggles: (Yes, I'll probably replace the color texture with a nicer (higher-resolution) one, this is just a first test...) |
- Resized feature ID texture to 16x16 to avoid upscaling. - Fix channel for feature ID texture: The ID was stored in channel 2 before, but is stored in channel 0 now. - Use larger "pixelated" image for actual texture
That was actually caused by the feature ID being stored in the wrong channel. This is fixed with the last commit. Also, changed the size of the texture to 16x16 to avoid upsampling. The actual color texture is now a larger, "artificially pixelated" image, like this: I think this makes sense, but I can also replace that texture with something that more resembles "a real house", if that is preferred. |
I have added some basic documentation for the first examples:
|
I have added a (preliminary) "complex" example that can (to some extent) be used for testing some of the changes from CesiumGS/cesium#10018 The example contains two classes, where the second one contains some of the "less trivial" types, namely
It also contains two feature IDs (an implicit and an explicit one). The https://github.com/javagl/3d-tiles-samples/blob/add-ext-mesh-features-samples/EXT_mesh_features/CommonSandcastleCode.txt has been updated with a basic toolbar box for selecting the From a first test, it basically ~"seems to work". Some changes have to be done for the finalization:
|
The last commit adds a "Complex Types" example, showing...
I'll probably trim down the "Complex Example" that I added earlier, to omit the array types that had been used there, so that it solely serves as a dedicated sample for multiple classes. |
The last commit brings this into a state that could be (close to) ready for a first review. The main change is that I added documentation, as a
The "Example Sandcastles" are still missing for most of them (except the |
The more I tried to "clean up" the I now changed it to focus on the Roughly speaking, there are two options:
I'm a bit on the fence here:
Any preferences? |
A dropdown list is more convenient for browsing the samples. And if the code is mostly the same it wouldn't be worth duplicating for each sample. So my preference is the second option. |
The common sandcastle code is now part of a "top-level" README - where "top-level" refers to the The "entry point" for these samples would therefore be https://github.com/javagl/3d-tiles-samples/tree/add-ext-mesh-features-samples/EXT_mesh_features |
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.
A couple cosmetic suggestions:
- The default camera view is too zoomed in, making it hard to see what's going on
- The city landscape is a bit gritty. I usually like choosing a nice grassy area as the default location for samples
The swath of examples is great, especially as debug/test data. Over time we should create more samples somewhere between debug data and real world data, but that's out of scope for this PR.
I did a high level pass over the documentation and samples and they generally looked good. Just a few minor comments from me.
|
||
## Example Sandcastle | ||
|
||
This example can be viewn with the [common `EXT_mesh_features` sandcastle](../#common-sandcastle-code) |
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.
This example can be viewn with the [common `EXT_mesh_features` sandcastle](../#common-sandcastle-code) | |
This example can be viewed with the [common `EXT_mesh_features` sandcastle](../#common-sandcastle-code) |
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
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.
Remove whitespace just to tidy up the document
"extensionsUsed" : [ "3DTILES_content_gltf" ], | ||
"extensionsRequired" : [ "3DTILES_content_gltf" ] |
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.
While not strictly necessary, it would be good practice to list out the glTF extensions like in https://github.com/CesiumGS/3d-tiles/tree/main/extensions/3DTILES_content_gltf#extension-json
0.0, 0.01, 0.0, | ||
0.0, 0.0, 0.5] | ||
}, | ||
"geometricError" : 1.0 |
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.
Usually leaf tiles have geometricError
of 0.0
|
||
The structure of the metadata is defined with an [`EXT_mesh_features` schema](https://github.com/CesiumGS/glTF/tree/3d-tiles-next/extensions/2.0/Vendor/EXT_mesh_features#schema-definitions) that contains a single class. The class only contains a single property, called `example_VEC3_FLOAT32`. The type of this property is a 3D vector with 32 bit floating-point components, as indicated by the type `"VEC3"` and the component type `"FLOAT32"`. | ||
|
||
### Metadata Instances |
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 believe we are calling these "entities", though I sometimes find myself gravitating towards "instances"
The inlined comments should be addressed with the last commits (each referring to one comment, applied to all README/tileset files - I hope that I didn't overlook anything).
That also bugged me, but I thought that "whatever magic
The problem (for me) with choosing a location is: There are so many of them :D So I picked the location of (what I think is) the Cesium Office. How about the fountain at the center of the Washington Square, If it should be some completely neutral place outside of the city, just drop the |
I like the Washington Square location 😄 |
Depending on whether there will be a "new" set of samples (i.e. a new PR), or this one will be updated, I'd use that location then accordingly. |
These correspond roughly to the EXT_mesh_features samples from CesiumGS#40, separated into EXT_mesh_features and EXT_structural_metadata samples, and updated according to the latest specification
Yes, #47 should contain everything that was contained here (the 'Feature ID by GPU instance' mentioned in #40 (comment) was not yet integrated here, but is still on an implicit TODO list) |
This is an initial DRAFT of some examples for
EXT_mesh_features
.It is not yet decided whether these samples should be here, because they are not strictly speaking "3D tiles samples". Also, there are some kinds of possible samples for
EXT_mesh_features
that cannot sensibly be part of a tileset (for example, an empty asset that only defines an example schema). But they may serve the purpose of getting an idea of which samples could make sense, and in how far certain functionalities are already supported in CesiumJS.The current state of this PR is "bare", without any READMEs, screenshots or documentation. These will be added at a later point, as appropriate.
There currently are 5 assets (each wrapped into a trivial tileset) in https://github.com/javagl/3d-tiles-samples/tree/add-ext-mesh-features-samples/EXT_mesh_features . Each asset has the same structure: It consists of a single scene with a single mesh with a single primitive that just consists of 4 quads:
The flavors of this asset, reflected in the test cases, are:
Cases 1 and 3 do not work yet, because of
Model
EXT_mesh_features: handle feature IDs without a property table cesium#9884Case 2 seems to work, but the displayed values for the vectors is not correct. The vectors should have the values
The values that are actually shown look like there might be an issue related to endianness, but I haven't investigated it in detail yet. (I can only say that when adding an accessor for these values, the right values are displayed when inspecting the accessor in the VS Code glTF plugin - so they should probably be correct there)
Case 4 does not seem to work - it does not seem to find the feature IDs when they are defined implicitly.
Case 5 causes everything to collapse and splill out obscure error messages...
NOTE: Each and every error might be an issue with the sample data itself ... or with CesiumJS. This has to be figured out as we move on. But particularly the last example is "relatively complex", and it's not unlikely that it is invalid in one way or another...
In order to quickly tests this, a first draft of the Sandcastle code (that would eventually go into the READMEs) is in https://github.com/javagl/3d-tiles-samples/blob/add-ext-mesh-features-samples/EXT_mesh_features/CommonSandcastleCode.txt
It contains a code block
that allows to quickly select the example that should be loaded, for debugging and testing.