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

Handle multiple feature ID sets in ModelExperimental #10018

Merged
merged 42 commits into from
Jan 28, 2022
Merged

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented Jan 19, 2022

Fixes #10007
Fixes #9894

This also partially addresses #9884, though picking is not currently supported for feature IDs without a property table, and the current picking system would need larger changes to address this (see #9884 (comment))

Summary of changes:

  • In ModelComponents, combined featureIdAttributes and featureIdTextures into one featureIds array. This is more consistent with the current EXT_mesh_features spec.
  • in ModelExperimental, replaced featureIdAttributeIndex and featureIdTextureIndex with featureIdIndex and instanceFeatureIdIndex, as the latter pair better match the glTF structure
  • FeatureIdPipelineStage now processes all the feature ID attributes instead of only the selected one. It populates a FeatureIds struct that can be passed into both feature ID selection (next bullet) and custom shaders.
  • SelectedFeatureIdStage handles the shader details for taking the selected feature ID set and populating a struct used by styling and picking. This is significantly easier now with the FeatureIds struct, it's simply selectedFeature.id = featureIds.SELECTED_FEATURE_ID (using a define to substitute in the right variable).
  • adds feature IDs to Custom Shaders via fsInput.featureIds.featureId_N or fsInput.featureIds.instanceFeatureId_N
  • updated the 3D Tiles Next Photogrammetry Sandcastle to use the new fsInput.featureIds.featureId_N syntax to avoid exposing private API.

To Do:

  • Update custom shader guide
  • Update documentation for feature ID stages
  • Fix broken unit tests
  • Add new unit tests
  • add featureIdIndex and instanceFeatureIdIndex to Cesium3DTileset for easier configuration
  • Update CHANGES.md
  • Check if instanced FEATURE_ID_n is added to the ProcessedAttributes correctly
  • fix race condition about featureIdIndex being set before the scene graph is constructed.
  • investigate styling, sometimes higher LODs aren't displaying the right color
  • Update other existing sandcastles
  • Investigate ModelColorPipelineStage DeveloperError
  • Investigate draco tile failures in Custom Shaders 3D Tiles sandcastle -> opened Fix regression in GltfLoader for handling Draco _BATCHID #10031

@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@ptrgags ptrgags marked this pull request as draft January 19, 2022 21:36
@javagl
Copy link
Contributor

javagl commented Jan 22, 2022

Just a short note: I think that http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles%20Next%20S2%20Globe.html&label=3D%20Tiles%20Next is still using the (now no longer working, internal) FEATURE_ID_TEXTURE approach for the custom shader.

@ptrgags ptrgags marked this pull request as ready for review January 24, 2022 20:54
@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 24, 2022

@lilleyse this PR is now ready for review. I do still want to add a sandcastle to show multiple feature Id sets once you finish updating the Ferry Building tileset and upload it to ion

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 25, 2022

@lilleyse I updated the 3D Tiles Next Photogrammetry Sandcastle with new styling and custom shader examples

Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be in pretty good shape overall. Here's my first round of feedback, I plan to test some examples next.

Source/Scene/Cesium3DTileset.js Show resolved Hide resolved
Source/Scene/ModelExperimental/CustomShader.js Outdated Show resolved Hide resolved
Source/Scene/ModelExperimental/PickingPipelineStage.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor

lilleyse commented Jan 25, 2022

@javagl just to confirm, have you tested all the samples in CesiumGS/3d-tiles-samples#40 on this branch?

Here's the testing strategy I was going to go with. Are there any that can already be checked off?

  • Test picking feature without corresponding property table (expect it not to pick anything until #9884 is addressed)
  • Test setting the active feature id and then picking and styling
  • Test setting the active instance feature id and then picking and styling
  • Test picking pnts with BATCH_ID
  • Test picking pnts with per point properties (expect picking to work now, but maybe not in the future #10025)
  • Test picking i3dm
  • Test picking b3dm
  • Test picking glTF with feature ID attributes
  • Test picking glTF with feature ID textures
  • Test picking glTF with implicit feature IDs
  • Test picking glTF with instance feature ids
  • Test picking glTF with implicit instance feature ids
  • Test custom shader for any of the above

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 26, 2022

Addressed the concrete feedback. Next step is investigating a bug @lilleyse pointed out to me where switching between a style and a custom shader leaves the tileset in a bad state:

Peek 2022-01-25 15-43

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 26, 2022

@lilleyse this is ready for review again.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 26, 2022

merging in the let/const changes didn't seem to work properly... I'm going to reset my branch and try again

@lilleyse
Copy link
Contributor

The new sandcastle option Multiple Feature ID Sets might be a little too busy with the color scheme. Maybe it's better to go with something simpler, like tinting roof windows yellow and facade windows purple.

@lilleyse
Copy link
Contributor

Aside from that, the recent changes look good. @javagl once you're satisfied with your testing we can get this merged.

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 27, 2022

@lilleyse I simplified the sandcastle like you suggested
image

@javagl
Copy link
Contributor

javagl commented Jan 27, 2022

I did some tests, roughly based on the bullet points above, but with some disclaimers (summarized below)

  • Test picking feature without corresponding property table (expect it not to pick anything until #9884 is addressed)

  • Test setting the active feature id and then picking and styling

  • Test setting the active instance feature id and then picking and styling

  • Test picking pnts with BATCH_ID

  • Test picking pnts with per point properties (expect picking to work now, but maybe not in the future #10025)

    • I think these are working as expected. When loading the cesium/Apps/SampleData/Cesium3DTiles/PointCloud/PointCloudBatched in my test sandcastle, it shows the "feature ID" (aka Batch ID) and some per-point properties in the Tooltip, if one manages to move the mouse directly over that single pixel :D
  • Test picking i3dm

  • Test picking b3dm

    • These also seem to work, based on the Instanced/InstancedWithBatchTable and Batched/BatchedWithBatchTable sample data...
  • Test picking glTF with feature ID attributes

  • Test picking glTF with feature ID textures

  • Test picking glTF with implicit feature IDs

  • Test picking glTF with instance feature ids

  • Test picking glTF with implicit instance feature ids

  • Test custom shader for any of the above


Disclaimers:

  • In general, I wasn't sure about the interplay of metadata and styling. For example, there is this line with color: "color(${color})" in one of the samples, and I don't know what color is. (No not that one, the other one. I mean the innermost one...)

  • Setting a style with a pointSize: 10.0 does not seem to have any effect:

    tileset.style = new Cesium.Cesium3DTileStyle({
      pointSize: 10.0
    });
    

    Is this a known issue?

  • Everything related to instance IDs: I'll have to look into that, in terms of example data and usage.

  • Custom shaders: Also would need a bit more time here to try out "things"...


General:

I've been testing most of this based on the (sketchy) "Test Sandcastle" that may, at some point, be a "template" for the example sandcastles that will go into the READMEs for the samples.

There are several issues with that:

Right now, accessing the metadata generically is ... clumsy: It has to do many instanceof/undefined checks, and a lot of "implicit, specific knowledge" is carved into this code. It might be that there are some abstractions in the latest state of this PR. If some of that code could be simplified, that would be great.

There is an experimental function for "Setting the active feature ID", but ... I have no idea how to know the actual number of available feature IDs in the first place. More generally: I assume that the concept of having an "active feature ID" is a tribute to having to transport this information through the rendering process into "the picking world". I could question the now 54 (fifty-four!) properties in the options for a tileset, and ask who is going to create a "mental model" of what they mean and what values they should have. But even if it is OK to just add more properties there, this is part of a "global state" (global in terms of the tileset), and I see this as a (possibly severe) limitation. What if someone wants to do styling based on one feature ID, and show a tooltip based on another feature ID? (Related to that: The ambiguities between "feature ID" and "instance feature ID" are ... not so nice. Conveying the difference and their meaning in context, in some sort of documentation/tutorial, may be difficult...)

It might be that these issues are already resolved by more recent changes. So to put it that way: If somebody could improve (as in: simplify and generalize) this test code at https://github.com/javagl/3d-tiles-samples/blob/add-ext-mesh-features-samples/EXT_mesh_features/CommonSandcastleCode.txt , that would be great...

@ptrgags
Copy link
Contributor Author

ptrgags commented Jan 27, 2022

@javagl a couple things to point out:

@lilleyse
Copy link
Contributor

I finished up testing. All looks good.

  • Test setting the active instance feature id and then picking and styling
  • Test custom shader for any of the above
  • Test picking glTF with instance feature ids (Sandcastle)
  • Test picking glTF with implicit instance feature ids (Same sandcastle as above)

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.

Handle multiple sets of feature Ids in ModelExperimental Adding feature IDs to custom shaders
4 participants