Apply texture transforms to property textures

[javagl]

A DRAFT addressing #11708

With this change, it appears to transform the texture properly:

This is a first shot (I haven't done anything in the CesiumJS+Metadata+Shader area before), and not yet tested extensively. Maybe someone wants to have a look whether this might go in the right direction. (If it does, the same will probably have to be done for feature ID textures - to be confirmed)

---

• 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 update the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[ptrgags]

@javagl I think this approach looks good! I left a couple comments explaining why MetadataPipelineStage works a bit differently than MaterialPipelineStage, and some ideas for unit testing.

[javagl]

The *PipelineSpecs seem to mainly focus on whether the "ShaderBuilder" generates the right text. I could probably add a few checks there. Some sort of "functional" test would be nice, though.

I tried this...

Maybe consider making a render test in ModelSpec that would render e.g. green if the transform is applied correctly but red if the transform was omitted?

... but... what to check there? I added test to render the model (and it took me a few hours and some swearing before I resorted to just restoring the default camera orientation after the "fly-to"), and it renders the "invalid" areas (which should not be visible after the transform) in 'red' with a custom shader. But it is not clear what that "RGB value" should be that is checked there. (And... using/setting debugCanvasWidth will break this check anyhow, because it will always look at the "upper left pixel" of the rendered image....). I'll try to find a sensible solution there. Something like expect(allRenderedPixels.toNotBe(red)) could do it, but does not seem to be used anywhere until now.

[javagl]

I found it a bit difficult to sensibly structure the test in the ModelSpec.js.The fact that there is no "convenience function" like
expect(magicallyFetchTheTextureValueAt(x,y)).toEqual(expected);
makes this feel a bit brittle.

What it is doing now:

• There is that test model with the unit square (as shown in the issue):
  
• It creates a shader that returns "RED" for "low" and "high" values (i.e. for the values that should be "moved out" of the unit square area by the texture transform
• The rendered square without the bugfix will look like this:
  
• It is moving the camera at a very specific location in the upper left (at (0.1, 0.1), with a distance of 0.15), meaning that the result of that 1x1-pixel rendering should be "completely red"

It "works" insfofar that this test...

• fails without the fix
• passes with the fix

which might be "enough". But maybe someone has ideas for improving that. The relevant spec starts at https://github.com/CesiumGS/cesium/pull/11709/files#diff-7118f1e2b4539329dcd5314bc1898c6007565fe4e096211621aef6be3d16543dR722

---

Running the tests locally results in a bunch of failed tests, e.g.

Scene/Scene > pickPosition returns undefined when useDepthPicking is false
DeveloperError: normalized result is not a number

or

Scene/Pick > sampleHeight > samples height from the globe
Expected 6343428.618064475 to be greater than 6356752.314245179.

(?!?!)

and a few timeouts. But this is unrelated to this change, and has to be tracked or addressed separately.

[javagl]

@ptrgags I tried to add some tests here

• The test in MetadataPipelineSpec is largely "copy-and-paste" from the other tests - the most specific part is that it checks that the uniform mat3 u_propertyTexture_0Transform; is present

• In line with your suggestion of

  Maybe consider making a render test in ModelSpec that would render e.g. green if the transform is applied correctly but red if the transform was omitted?

  I tried to add such a test in ModelSpec that zooms into a region of the quad that is 'red' when the transform is not applied, and then checks that this 1x1 pixel rendering is "not red". It feels a bit brittle, and does not check whether the access "returns the right property values" or so, but ... it 'fails' without the fix, and 'passes' with the fix, so this may be as good as it gets...

[ptrgags]

@javagl just a couple comments.

I'm going to try out the Sandcastle you posted in the original issue next. Do you have any other Sandcastles you've used while testing or just that one?

[ptrgags]

I tried your Sandcastle with the data from the zip file from the description in #11708, and confirmed that it renders the bottom right quadrant of the texture:

I also did a pass through all the metadata-related Sandcastles in the gallery, everything there is still working fine.

So this PR looks close, once you address my feedback above we can merge this.

[javagl]

I'm going to try out the Sandcastle you posted in the original issue next. Do you have any other Sandcastles you've used while testing or just that one?

I only tested this specific model with the one sandcastle. The intention here was to "zoom in" to what is wrong. (This is dangerous insofar that corner cases or "unusual" cases could be overlooked, but I hope that the scope of this PR is narrow enough for that not to happen...)

[javagl]

(EDIT: The question at the bottom was answered with 'Yes' in the meantime)

I think this could be merged. One thing that I'm not sure about:

In the follow-up PR for fixing the same issue for feature ID textures, it was necessary to pass incrementallyLoadTextures: false to one of the spec functions, at https://github.com/CesiumGS/cesium/pull/11766/files#diff-7118f1e2b4539329dcd5314bc1898c6007565fe4e096211621aef6be3d16543dR825

Here this does not seem to be necessary: https://github.com/CesiumGS/cesium/pull/11709/files#diff-7118f1e2b4539329dcd5314bc1898c6007565fe4e096211621aef6be3d16543dR753

I wonder whether there is a reason for that, or whether this is one of these cases where a "spurious test failure" is about to happen...

Should I just add the incrementallyLoadTextures: false here as well, "to be safe(r)"?

[ptrgags]

This looks good now, thanks @javagl for the updates!