fix: get feature info with projection

[zoran995]

Description

When using WMS in a WebMercatorProjection instead of a GeographicProjection, the user gets the wrong feature position in response. The PR fixes the issue by passing the tiling scheme projection as an argument to the callback, which is then used to unproject the result and set the correct feature position.

Issue number and link

Fixes #9363

Testing plan

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, @zoran995!

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

[mzschwartz5]

I don't think this is particularly robust. What if we introduce a new type of projection? Or what if a user has implemented their own projection based on the MapProjection interface?

I also don't think it's good practice to have featureInfo.position be instances of different types (Cartographic or Cartesian3) depending on which projection we're dealing with. Actually, I'm a little confused because, looking at the implementations of GeographicProjection.unproject and WebMercatorProjection.unproject, their jsdocs indicate they return Cartographic, not Cartesian3. I think you may have misused the unproject method here.

Side note- let's use scratch variables for methods like unproject which, otherwise, will needlessly allocate memory for the result. (See here in coding guide)

[zoran995]

Sorry, I guess I chose the wrong variable name for the projection.unproject result; it actually returns the Cartographic, which is expected behaviour in all cases for featureInfo.position. I have updated the variable name and added a scratch variable.

[mzschwartz5]

That's okay, but there's still the matter of making this change robust to different types of projections.

Also, even in its current state, does the logic here work for mercator projections?
const mercatorPosition = new Cartesian3(x, y, 0);

you're creating a Cartesian3 directly from Cartographic components (longitude and latitude), and then unprojecting to get... another Cartographic?

Are the original longitude and latitude not correct? If so, I think the fix should target whatever calculates those values originally; rather than patching the values in here by reverse-projecting.

[zoran995]

Here is the sandcastle @jjhembd created a few years ago in a related ticket that showcases how it is used.

When a user adds a WMS server with a web mercator tiling scheme, it will respond in mercator coordinates, not latitude/longitude. We use those to create cartesian3 and then convert them to cartographic.
Sample of raw WMS get feature info response

[
    {
        "type": "Feature",
        "id": "Unesco_point.108",
        "geometry": {
            "type": "Point",
            "coordinates": [
                1424421.9403,
                5155468.5755
            ]
        },
        "geometry_name": "the_geom",
        "properties": {
            "cod_unesco": "IT/VA_91bis",
            "sito": "Centro storico di Roma, le proprietà extraterritoriali della Santa Sede nella città e San Paolo fuori le Mura",
            "seriale": 1,
            "cod_compon": "091-002(b)",
            "componente": "Complesso della Santa Scala",
            "tipo_area": "sito"
        },
        "bbox": [
            1424421.9403,
            5155468.5755,
            1424421.9403,
            5155468.5755
        ]
    }
]

[mzschwartz5]

I see that makes sense. So the meaning of feature.geometry.coordinates depends on the projection used? I.e. a WMS may return web mercator coords for that field, or it may return long/lat (for a geographic projection)?

If that's the case, these changes need to either happen as part of the MapProjection interface, or maybe as part of WebMapServiceImageryProvider, so that they can be more robust to different types of projections / meanings of feature.geometry.coordinates

[kring]

No doubt there are things to improve here! And I don't want to discourage anyone from considering those improvements. But IMO it shouldn't hold up a community PR that is fixing a real issue in a reasonable way that is very unlikely (as far as I can see) to cause any new problems now or in the future.

Full disclosure: this PR is from my old team, and it's been fixed in TerriaJS for close to a decade now by simply supplying a custom GetFeatureInfo callback, as you suggested.

My two cents on some bigger improvements... First, this GetFeatureInfoFormat thing has always been a little optimistic. Pretty much every map tile service has its own way of providing feature information, so this idea that we can supply a single "default" that works across a bunch of them? It's mostly aspirational nonsense that maybe kind of works well enough some of the time. Fundamentally, each imagery provider needs to be responsible for retrieving and interpreting feature information. I don't think there's any MAGIC.convertTheseCoordinatesIntoWhatWeNeed(coordinates) that makes sense here.

Second, I kind of think GeographicProjection is broken. I originally added it for terrain and imagery a looong time ago, and I guess meters was convenient at the time. But it really doesn't make a lot of sense. We do this instanceof thing all over the code base, usually for just this reason:

• https://github.com/search?q=repo%3ACesiumGS%2Fcesium+%22instanceof+GeographicProjection%22&type=code
• https://github.com/search?q=repo%3ACesiumGS%2Fcesium+%22instanceof+WebMercatorProjection%22&type=code

So if I were going to change things, and it would absolutely be a breaking change, I'd probably make GeographicProjection's projected coordinates decimal degrees instead of meters. And provide separate methods for working with projected meters, if necessary. And then the special-case logic wouldn't be needed here anymore (and probably other places, too).

This all seems way outside the scope of this PR, though.

[javagl]

But IMO it shouldn't hold up a community PR that is fixing a real issue in a reasonable way that is very unlikely (as far as I can see) to cause any new problems now or in the future.

I generally agree, with the details depending on the scope of possible changes or refactorings. If this PR could be improved with little effort (roughly, avoiding the instanceof and making this part projection-agnostic, maybe by creating this "magic" function at the right place and just calling it there), then this could be worthwhile.

If this is just a symptom of a deeper conceptual issue (and what you said sounds like this is the case), then this PR could be merged, and the necessity for improving that part could be tracked in a new issue. This could link to this discussion and summarize some approaches for improvements.

(I know, "🎶 1500 open issues on the wall, 1500 open issues - create a PR and close one down, ... 1501 open issues on the wall". The fact that "refactor something to make things better"-issues are basically never addressed is an unrelated issue on a different level)

---

Fundamentally, each imagery provider needs to be responsible for retrieving and interpreting feature information. I don't think there's any MAGIC.convertTheseCoordinatesIntoWhatWeNeed(coordinates) that makes sense here.

I'm a bit confused. The first part sounds like it's nearly clear that this function should be part of the ImageryProvider class/interface. But how this is filled with life, for the specific ImageryProvider types, is what I referred to as the "degrees of freedom", and may be the crux here. (I still think that a key aspect is the connection between the tiling scheme and the GetFeatureInfoFormat thingy, but there are several layers of questions and unknowns for me)

---

We do this instanceof thing all over the code base, usually for just this reason:

I've seen this (and many related things that may be symptoms of what you meant with "GeographicProjection is broken" - including the ripple effect of rectangle.width being broken, which did cause quite some headaches for me...).

I sometimes try to intentionally take an overly idealistic/theoretical stance on certain aspects of OOP. A seemingly extreme form of that: Whenever there is some if (someCondition) doThis(); else doThat();, one should at least consider polymorphism. It could make sense to have a SomethingDoer interface with a doIt() method and two implementations, and replace that if-statement with a plain somethingDoer.doIt();. This is overly suggestive, and of course should not necessarily and inconsiderately be applied, but in (surprisingly) many cases, it does make sense.

Now,... instanceof is a very special form of such an if-decision, and there are very few cases where it does not make sense to replace that with some form of polymorphism. (With a few degrees of freedom here as well - some things are, at the core, so vastly different that it may be hard or impossible to define a sensible common interface...)

[kring]

The first part sounds like it's nearly clear that this function should be part of the ImageryProvider class/interface.

It is part of the interface, via the properly polymorphic pickFeatures method:
https://github.com/CesiumGS/cesium/blob/1.134.1/packages/engine/Source/Scene/ImageryProvider.js#L218

Then there's this whole weird, caveat ridden, GetFeatureInfoFormat thing that can be used to implement that method.

rectangle.width being broken

Yeah the GeographicTilingScheme's "native" coordinate space is longitude/latitude degrees. Different from the GeographicProjection's projected coordinate system (meters) and Cesium's usual internal units (radians).

You certainly shouldn't assume a "native rectangle" would be in radians. That's kind of the whole point of this concept of native versus non-native. But the fact that the projection and the tiling scheme aren't consistent is pretty weird.

Now,... instanceof is a very special form of such an if-decision, and there are very few cases where it does not make sense to replace that with some form of polymorphism.

Agreed. We just have the annoying situation here where project / unproject are supposed to be the polymorphic methods that make sense here. Except in the case of GeographicProjection we can't rely on them because the implementation is known to be broken (by using unsuitable units).

[javagl]

The documentation of the Rectangle class says that it is in radians. The implementation of things like width silently assumes that the values are in radians (and just returns wrong results otherwise). At no point in the codebase anyone can know whether a Rectangle is stored in the "right" or "wrong" coordinates, or whether any function accepts or returns one of these "right" or "wrong" rectangle.

tl;dr: There should be a CartesianRectangle (and Rectangle should probably be called CartographicRectangle).

But this, as well as any attempts to change the GeographicProjection or what pickFeatures is doing, involves an analysis and refactorings that would have to be stretched out over several months.

[kring]

Good point. FWIW, in cesium-native, we have a GlobeRectangle (always radians, handles anti-meridian) and a Rectangle (unspecified units, always minX <= maxX).

[mzschwartz5]

Sorry- I was wrong about needing to use a scratch variable. It's actually going to cause every featureInfo to have the same position. I didn't look closely enough at the usage, but we do need to allocate each new position as you were originally doing. Mea culpa.

[zoran995]

No worries, I also missed the point that all feature info positions will share the same reference.

Updated