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

Add base color texture node to tileset materials #406

Merged
merged 3 commits into from
Oct 19, 2023

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Jul 31, 2023

Follow up to #394 and builds upon #391

Adds a new MDL node cesium_base_color_texture that retrieves the base color texture of the tile. This can be used to create more complex materials than before like clipping out regions of a tileset. Here Cesium World Terrain is being clipped by the Cesium logo with clamp-to-edge sampling.

image

tileset-materials-complex.zip

(Make sure to set app.useFabricSceneDelegate = false and enable fabric when the popup appears, otherwise it may not recognize the custom material. This is a known issue with USD material population in Fabric.)

How it works:

If a material doesn't have any cesium_base_color_texture nodes it works the same as before: the Fabric prims reference the existing material and that's it.

If a material has cesium_base_color_texture nodes we duplicate all the prims in the material network, reconnect the prims, and add a prim to read from the base color texture. Since materials can't reference texture paths via primvars we can't share the material like before, and instead need to duplicate the material X number of times depending on the size of the material pool. This also means you can't tweak materials dynamically unless you're able to watch for changes and update all materials in the pool.

Future work:

  • Figure out how to hide cesium_base_color_texture inputs
  • Update materials in the pool when the main material changes
  • Delete material pools that are no longer in use. This is especially important now that the tileset material is part of the material definition.
  • Add other cesium nodes like cesium_base_color_factor, cesium_base_color_texture_alpha, cesium_emissive_factor, etc. We can expose whatever material values we want, I just didn't want to clutter this PR.
  • Add MDL node to lookup color from an imagery layer. Support for multiple raster overlays #84 would need to be implemented first. This would let us do more complex things like water materials using the CWT water mask.

@weegeekps
Copy link
Contributor

(Make sure to set app.useFabricSceneDelegate = false and enable fabric when the popup appears, otherwise it may not recognize the custom material. This is a known issue with USD material population in Fabric.)

Should we create a temporary app that is derived from our test app that sets this to false while this is broken? It seems like it would make launching from our editors easier, but maybe it's overkill?

@r-veenstra
Copy link
Contributor

It appears that if I try to switch between materials, or remove the material entirely, the tileset gets stuck with the first custom material I applied. It appears to only happen to the final level of detail that gets loaded, as the tile reloads you can see the new material coming in to the coarser detail levels, but it still ends up with the old material. Best shown in this capture.

Screen.sharing.-.2023-08-02.10_02_15.AM.mp4

This was referenced Aug 2, 2023
@lilleyse lilleyse changed the base branch from cesium-material-graph-nodes to remove-empty-pools August 2, 2023 20:26
@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 2, 2023

@r-veenstra I fixed the bug with switching materials. It was a bug in UsdNotificationHandler. When a material is added to a tileset for the first time it's assigned the MaterialBindingAPI schema which triggers a resync, which triggers a second OmniTileset to be added. The fix was to not add another OmniTileset when that happens. 0dcb35e

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 2, 2023

Should we create a temporary app that is derived from our test app that sets this to false while this is broken? It seems like it would make launching from our editors easier, but maybe it's overkill?

Or we could just disable fabric by default. Though that would mean having to click yes on the popup every time...

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 3, 2023

I still need to fix the texture lookup functions to support y-up and z-up (probably via a uniform), and then this should be ready for review.

Edit: I may tackle "Update materials in the pool when the main material changes" in a follow up PR

@weegeekps
Copy link
Contributor

Should we create a temporary app that is derived from our test app that sets this to false while this is broken? It seems like it would make launching from our editors easier, but maybe it's overkill?

Or we could just disable fabric by default. Though that would mean having to click yes on the popup every time...

I suspect this would break unit testing.

Base automatically changed from remove-empty-pools to main August 3, 2023 15:10
@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 3, 2023

I added a uniform called up_axis for selecting between y-up and z-up.

y-up-z-up

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 3, 2023

A couple changes to the mdl. Existing .usda's will need to be updated.

  • Renamed min_world_xy to min_world
  • Renamed max_world_xy to max_world
  • Renamed lookup_world_texture_float4 to cesium_lookup_world_texture_float4
  • Renamed lookup_world_texture_float3 to cesium_lookup_world_texture_float3
  • Renamed lookup_world_texture_color to cesium_lookup_world_texture_color

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 3, 2023

@weegeekps this is ready for review now. For testing you can check out the .zip posted in the PR description.

@lilleyse lilleyse mentioned this pull request Aug 15, 2023
@lilleyse lilleyse force-pushed the tileset-material-nodes branch from 0f6dbca to 993e8f2 Compare August 16, 2023 13:53
@lilleyse lilleyse marked this pull request as draft August 16, 2023 14:06
@lilleyse
Copy link
Contributor Author

I converted this to a draft. I'm experimenting with a way for tiles to share a single material, and depending on how that goes the approach here may change.

@lilleyse lilleyse force-pushed the tileset-material-nodes branch 2 times, most recently from 6006330 to d7fea97 Compare October 19, 2023 13:18
@lilleyse lilleyse changed the base branch from main to fix-debug-color October 19, 2023 13:19
@lilleyse lilleyse force-pushed the tileset-material-nodes branch 4 times, most recently from 2c20a58 to 1e9dd7e Compare October 19, 2023 19:02
@lilleyse
Copy link
Contributor Author

This is ready for review again. The basic approach is still the same, but I changed what MDL functions are exposed.

  • cesium_base_color_texture_float4: returns the base color texture as a float4
  • cesium_imagery_layer_float4: returns the imagery layer at the given index as a float4
  • cesium_material: a simple wrapper around gltf_material. This is useful since gltf/pbr.mdl is not automatically included in the omniverse MDL search paths.

Together you can create an MDL like this:

material-graph-example

imagery-layer-blending-test.usda.zip

@lilleyse lilleyse marked this pull request as ready for review October 19, 2023 19:10
@lilleyse lilleyse force-pushed the tileset-material-nodes branch from 1e9dd7e to 7785d94 Compare October 19, 2023 20:04
Base automatically changed from fix-debug-color to main October 19, 2023 23:01
@lilleyse
Copy link
Contributor Author

@corybarr you can ignore the sample data in the PR description. Some of those nodes have been removed temporarily, to be added back in a later PR. See the sample data in #406 (comment) instead.

@@ -26,6 +27,7 @@ FabricMaterialDefinition::FabricMaterialDefinition(
_hasVertexColors = materialInfo.hasVertexColors;
_hasBaseColorTexture = hasBaseColorTexture;
_imageryLayerCount = imageryLayerCount;
_tilesetMaterialPath = tilesetMaterialPath;
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit, but I'd probably do this and the above line as an initializer list. Any thoughts on what to do for consistency or other reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 67024a8

Copy link
Contributor

@corybarr corybarr left a comment

Choose a reason for hiding this comment

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

Again, OV MDL isn't an area I've spent a lot of time in. Duplicating and reconnecting is an interesting solution. Code makes sense and LGTM.

@lilleyse lilleyse enabled auto-merge October 19, 2023 23:30
@lilleyse lilleyse merged commit 5ec6e4b into main Oct 19, 2023
3 checks passed
@lilleyse lilleyse deleted the tileset-material-nodes branch October 19, 2023 23:50
@lilleyse lilleyse mentioned this pull request Nov 9, 2023
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.

4 participants