search

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

Rename imagery to raster overlay #641

Closed corybarr closed 7 months ago

corybarr commented 7 months ago

Resolves #612

This is fundamentally simple. By and large, any "imagery" or related string has been changed to "raster_overlay" or a casing variant.

There are still 25 instances. These are exclusively in .md documentation and a few user-facing strings in Python code. Sometimes "raster overlay" sounded overly technical for end users. This is debatable.

Previously, we had Imagery and ImageryLayer in variable and class names. I kept that. It leads to long variable names. The worst is overlayRasterOverlayLayerCount, but after first trying it without "Layer" I think this is best.

lilleyse commented 7 months ago

Previously, we had Imagery and ImageryLayer in variable and class names. I kept that. It leads to long variable names. The worst is overlayRasterOverlayLayerCount, but after first trying it without "Layer" I think this is best.

I slightly prefer without "Layer", but since it doesn't impact the USD schemas or UI I don't see a reason to hold up the PR.

corybarr commented 7 months ago

Previously, we had Imagery and ImageryLayer in variable and class names. I kept that. It leads to long variable names. The worst is overlayRasterOverlayLayerCount, but after first trying it without "Layer" I think this is best.

I slightly prefer without "Layer", but since it doesn't impact the USD schemas or UI I don't see a reason to hold up the PR.

I found it helpful to have different variables names for raster overlays at the USD level and those in the material. Otherwise you get rasterOverlayPath, which can ambiguously refer to the prim path or MDL node path. All the "layer" symbols deal with the objects instantiated for the material. But "layer" doesn't imply that. Maybe we could swap the "layer" string with something material-specific. Maybe "Node."

lilleyse commented 7 months ago

I don't think the ambiguity is the big deal. Maybe just make the change in a new PR and see how it feels?

Also I realized that some of these changes are user facing like cesium_raster_overlay_layer_float4 so the simpler terminology is definitely preferable there.