CesiumGS / cesium-omniverse

Bringing the 3D geospatial ecosystem to Omniverse
https://cesium.com/platform/cesium-for-omniverse/
Apache License 2.0
55 stars 7 forks source link

Cartographic Polygon editing crash - fabric only #632

Closed r-veenstra closed 7 months ago

r-veenstra commented 7 months ago
  1. Start USD Composer (I used 2023.2.1) - build from commit d1b7ebcbdcd0020fca64eb21cc8a19ed3abe6a9d
  2. Enable fabric
  3. Add a Cartographic Polygon via the quick add menu
  4. Select it
  5. Click Edit Control Vertices
  6. Click on the default ground plane to place a first point
  7. OV should briefly hang then crash

Note: Does not crash when fabric disabled

cc @corybarr

lilleyse commented 7 months ago

This doesn't seem to happen for regular BasisCurves.

Maybe a reason for CesiumCartographicPolygon to have a rel to a BasisCurves prim instead of being a subclass.

lilleyse commented 7 months ago

Actually, it seems like both BasicCurves and CesiumCartographicPolygon are working in USD Composer 2023.2.2.

Still might be worth considering the rel approach if that improves the workflow or fixes other bugs.

CC https://github.com/CesiumGS/cesium-omniverse/pull/630 and https://github.com/CesiumGS/cesium-omniverse/issues/636

corybarr commented 7 months ago

This doesn't seem to happen for regular BasisCurves.

Maybe a reason for CesiumCartographicPolygon to have a rel to a BasisCurves prim instead of being a subclass.

Moving to a rel would also let us query any prim that has a points attr. I can image some hypothetical dataviz workflows where creating a BasisCurves would be more work than needed. Users wouldn't necessarily have to even know about a BasisCurves.

r-veenstra commented 7 months ago
r-veenstra commented 7 months ago

@corybarr @lilleyse I can confirm that the original repro steps from this issue no longer cause a crash in 2023.2.2 with latest main 57fdeeebf7c47ed08282d981056be98eb72c49cb

r-veenstra commented 7 months ago

However, if I run the repro steps, save a USDA, and re-open the usda, I do get a crash

So it seems the issue is currently related to editing CesiumCartographicPolygons that have been loaded from a USDA, vs being freshly created.

corybarr commented 7 months ago

That's puzzling. It's strong evidence that we won't be able to fix the issue in our codebase, but let me see if I can did up any more precise info in some tests.

corybarr commented 7 months ago

I tested this with the absolute minimal inheritance example:

class CesiumBasisCurvesChildPrim "CesiumBasisCurvesChildPrim" (
    doc = """Test prim for inheritance crashes"""
    inherits = </BasisCurves>
    customData = {
        string className = "BasisCurvesChild"
    }
) {
}

With a raw BasisCurves I was able to create this with FSD enabled:

image

With a BasisCurvesChild it crashed. Aside from executing the prim's Define via our UI, the example bypasses our codebase. I think we should provide this minimal example to NVIDIA so they can reproduce.

corybarr commented 7 months ago

In the meantime, let's fall back to a rel. Agreed? @lilleyse @r-veenstra

r-veenstra commented 7 months ago

A rel that points to a BasisCurve with a GlobeAnchor added, yes?

corybarr commented 7 months ago

@r-veenstra Exactly

r-veenstra commented 7 months ago

@corybarr makes sense. It should also resolve https://github.com/CesiumGS/cesium-omniverse/issues/636