Proper handling of offset and scale in metadata picking

[javagl]

Description

Addresses #12236

The metadata picking did not take into account the offset/scale that may be defined in a class property or property texture property.

The solution is to ensure that the offset/scale (and normalization) are properly handled when converting the metadata values into values that are supposed to be written into the metadata picking frame buffer. For example, a normalized UINT8 property with offset=1.0 and scale=2.0 will be converted into 1.0 + 2.0 * ~0.5 = 2.0 by the standard metadata handling, But before writing this into the frame buffer, it has to be converted back into the [0, 1] range (reflecting the [0,255] range that can be stored in the frame buffer).

After reading the value from the frame buffer, it has to be converted back into the proper metadata value. These operations strongly resemble the MetadataComponentType.apply/unapplyValueTransform, but have additional caveats

• Every value must be mapped to the [0, 1] range
• This mapping is not done by "implementing it", but by generating shader code that implements it
• An offset/scale that is defined by a 'class property' may actually be overridden by the 'property texture property'

The last point may make it necessary to store the "effective" offset/scale inside the PickedMetadataInfo. Right now, it only stores the MetadataClassProperty. But when the offset/scale have been defined in the PropertyTextureProperty or a PropertyAttributeProperty (!), then these offset/scale values have to be used for the conversions.

Issue number and link

#12236

Testing plan

Details TBD - started creating specs

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

[javagl]

It's never as easy as it looks...

The last point may make it necessary to store the "effective" offset/scale inside the PickedMetadataInfo.

This is not possible. At the point where the actual value is obtained (namely, inside the shader), it is not longer possible to know where any offset/scale came from to begin with. So it's possible to "undo" the value transform in the shader (with some quirks). And it is possible to write that value in [0,1] into the color (ending up as [0, 255] in the frame buffer). But it is not possible to convert this back to the right value after reading it from the frame buffer, because at that point, we don't know whether the "value transform" that was applied came from a 'class property' or from the 'property texture/attribute property'.

There might be solutions to that, but all approaches that I can think of right now are ... complicated, to say the least. We certainly don't want to replicate the whole logic of value transforms (and things like KHR_texture_transform) inside the metadata picking shader function ... 🤔

[javagl]

The solution to the previous point turned out to be not toooo complicated.

When the picking starts, it will obtain the StructuralMetadata from the object, and then either obtain the PropertyTextureProperty or the PropertyAttributeProperty that corresponds to the picked metadata. (This may require some more thought - e.g. could there be an attribute and a texture that describe the same property...? Just thinking about corner cases and limitations here...).

This object always offers the offset/scale, in its final form - i.e. even when no offset/scale have been defined, this will be filled with default values, but also handle possible overrides from the class property. This object is then put into the PickedMetadataInfo, and used

1. for creating the shader code that maps the metadata values to [0, 1] for the picking
2. for converting the value in [0,255] from the frame buffer back into its original range

I started adding the corresponding tests. In that process I stumbled over an issue where the shader compilation bails out for normalized UINT8 array properties in property textures with offset/scale being defined in the class property, and it looks like this is independent of the picking (and therefore may have to be moved into a dedicated issue), but I have to verify that.

I'd also like to do more tests with property attributes (because these are currently not covered at all). Since the picking only relies on the metadata, and this does not even "know where it comes from" (attribute or texture), this might just work transparently. But now there is some code that explicitly has to handle both textures and attributes, so that should be covered by specs as well.

[javagl]

So, I did try out some of this with property attributes. Or rather tried to try it. There are more issues with that, and see the handling of property attributes for metadata picking as a part of #12225 , likely to be spun out into a dedicated issue. (Handling of arbitrary types being moved through the frame buffer does raise quite a few questions).

So I'll consider this as ready for review, given that it contains several spec test cases, and I tried it out with the data from #12232 (where this originally came up), and there, the current state seems to work as expected.

I did have to re-run some CI steps a few times. We do have some flaky tests there...

[javagl]

A small update: As mentioned above, picking arbitrary property attributes is not supported until now. It is not entirely clear what the expected behavior should be when someone tries to pick property attributes. But in the previous state, it could happen that this caused an error to be thrown (specifically, when the property attribute type was one of the types that could not trivially be transported through the picking frame buffer, like a FLOAT32 or ENUM).

The last commit handles this: The function that determines the "picked metadata property" now only looks for property texture properties (and no longer for property attribute properties), and bails out early if no property texture property is found. A // Note... was added to the function, pointing to #12225 , which keeps track of this.

[ggetz]

Hi @javagl, I'm taking a look at this PR now. Do you have an updated testing plan that I can use to verify the picked vaues?

[ggetz]

Suggested change

	/**
	* The the `PropertyTextureProperty` or `PropertyAttributeProperty`
	* that is described by this structure, as obtained from the
	* property texture or property attribute of the `StructuralMetadata`
	* that matches the class name and property name.
	*
	* The `PropertyTextureProperty` and `PropertyAttributeProperty`
	* types are not public and do not share a common base type.
	* But they both define the getters
	* - `hasValueTransform` (`boolean`)
	* - `offset` (`any`)
	* - `scale` (`any`)
	*
	* @type {object}
	*/
	this.metadataProperty = metadataProperty;
	/**
	* The the metadata property
	* that is described by this structure, as obtained from the
	* property texture or property attribute of the `StructuralMetadata`
	* that matches the class name and property name.
	*
	* @type {PropertyTextureProperty|PropertyAttributeProperty}
	*/
	this.metadataProperty = metadataProperty;

Much of this is redundant when specifying type.

[javagl]

Related to CesiumGS/3d-tiles#650 ...

If the hasValueTransform/offset/scale were defined in some common TransformableValue interface, then this would be sufficient. But as it stands, PropertyTextureProperty and PropertyAttributeProperty are distinct, unrelated classes, and it is not clear whether the consumers of the metadataProperty will ever try to access the textureReader of the former.

I think that it is important to know what the users of this class rely on. But if desired, I can remove that information.

[ggetz]

I agree with the sentiment. Class-design aside, that should the point of providing type information.

[javagl]

So... I assume that this is resolved...?
(If the solution is "REMOVE IT!", then you may have to write "REMOVE IT!" - I'm not soooo good at this 😆 )

[ggetz]

Sorry for the ambiguity: REMOVE IT! 😆

[javagl]

Unresolving:

I stumbled over this occasionally. For the suggested change and the resulting build error at https://github.com/CesiumGS/cesium/actions/runs/11584611448/job/32252079758?pr=12237#step:6:2897 , there are basically the two-and-a-half options:

• Refer to the type without including it (causing the current error)
• Include the type (which causes a linting error)
• Do not refer to the type - which defeats the purpose of the type information

@ggetz Any pattern of how to solve this?

[ggetz]

I think this is occurring because PropertyTextureProperty are PropertyAttributeProperty are marked as private. (They are also both marked as experimental, and that isn't necessary if the API is private.) Otherwise, there should be no need to import the class for the typing to work.

When we take a pass on the metadata typing, we should probably check to make sure private classes are appropriately marked as such or not.

Ok to skip for now.

[ggetz]

Since this "type" is used repeatedly, I would suggest using typedef to define it fro both convenience, accuracy, and specificity.

For instance, I think that MetadataPicking.convertToObjectType has incomplete types for the value parameter.

And Scene.prototype.pickMetadata could be updated to use this more specific type instead of any.

Something like:

/**
 * A raw value or array representation of a metadata value
 * @typedef {(number|string|boolean|number[]|undefined)} MetadataType.packedValue
 */

I maybe would suggest doing the same for the instance values, like:

/**
 * An instance of a metadata value
 * @typedef {(number|string|boolean|Cartesian2|Cartesian3|Cartesian4|Matrix2|Matrix3|Matrix4|undefined)} MetadataType.instanceValue
 */

[javagl]

This is somehow related to #12075 (comment)

The type information for metadata is difficult. You mentioned
(number|string|boolean|Cartesian2|Cartesian3|Cartesian4|Matrix2|Matrix3|Matrix4|undefined)

But in reality, in many places, it would be something like

(bigint|number|string|boolean|Cartesian2|Cartesian3|Cartesian4|Matrix2|Matrix3|Matrix4|bigint[]|number[]|string[]|boolean[]|Cartesian2[]|Cartesian3[]|Cartesian4[]|Matrix2[]|Matrix3[]|Matrix4[]|undefined)

(Give and take a few, depending on the subtle limitations here and there that are highly specific for the context (e.g. for property textures, or implementations that are "incomplete" in terms of types that actually are supported))

It would be unfortunate if this tempted people to not even trying to document things properly, but just use the

/**
 * @private
 */

that currently appears 349 times in the codebase.

I'd like to be more specific. And I can try to be more specific. But thoughness takes time...

[ggetz]

No need to fix this everywhere in the code base for this PR. Would it be sensible to create the relevant type definitions with our best list of possible types and use them in the scope of this PR?

We can submit a cleanup issue that can cover the remaining cases.

I'm harping on this because the types are exposed through a public API, and the typings will help users of the library, especially those using typescript.

[javagl]

Are you opposed to keeping the {any} in this (private!) part for now, and tracking possible refinements of the JSDoc for better TypeScript-consumabiltiy in a dedicated issue?

[ggetz]

I'd really like to see better typing, but if that would significantly delay merging this it can documented in an issue and be done in a follow-up.

[javagl]

I addressed most of the inlined comments, and added specs.

The main open point seems to be the type handling that was already brought up in #12075 (comment) . While it would be desirable to have stricter type information, the variety of metadata types makes it difficult to ~"quickly come up with" something, and I'd say that more thought has to go into that. (This is amplified by the fact that in the TypeScript world, these types are part of the API, and every change there is a breaking change).

In terms of testing: The newly added specs dedicatedly cover the case that there is an offset and scale.

I can try to create further samples, and it would actually be good to extend the existing 3d-tiles-samples with cases that use offset and scale (currently, they do not cover this at all). But the schedule for doing this within this PR may be a bit tight...

[javagl]

The build is failing at "verify batched features statistics". I've probably restarted this a dozen times already, and 3 times in this PR. We should disable this test.

[ggetz]

This is amplified by the fact that in the TypeScript world, these types are part of the API, and every change there is a breaking change

Narrowing the types may be a breaking change, but I don't think adding possible types should be breaking. LMK if you disagree.

[ggetz]

The build is failing at "verify batched features statistics". I've probably restarted this a dozen times already, and 3 times in this PR. We should disable this test.

Unfortunately, the test failures due to #11958 are a race condition. It is not the failing test that is itself the problem, so the error will just happen in another test if we ignore that one.

We need to spend some time tracking that race condition down soon.