Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve name fallback for GeoJSON #11633

Open
mfarmer-ara opened this issue Nov 17, 2023 · 5 comments · May be fixed by #11637
Open

Improve name fallback for GeoJSON #11633

mfarmer-ara opened this issue Nov 17, 2023 · 5 comments · May be fixed by #11637

Comments

@mfarmer-ara
Copy link

mfarmer-ara commented Nov 17, 2023

We have files that are generated with empty title and descriptions, but a filled key called country-name.

Eg:

"properties": {
  "title": "",
  "description": "",
  "country-name": "North Carolina",
  "two-letter-code": "NC",
  "fill": "#f0a3FF",
  "fill-opacity": "0.5
 },

The GeoJsonDataSource file loops through and attempts to figure out what name property to use. If country-name comes before title, it uses that, but if it comes after it assumes title, which is empty.

Current behavior:

https://github.com/CesiumGS/cesium/blob/main/packages/engine/Source/DataSources/GeoJsonDataSource.js#L151C11-L164

Is it possible/would it be ok to add an additional check to:

  • Check if value of key is empty and if so, check the next key.

The preferred behavior is to show x-name even if it shows up after an empty title but only if the title is empty.

@ggetz
Copy link
Contributor

ggetz commented Nov 20, 2023

Thanks for the proposal @mfarmer-ara!

It looks like there is no defined behavior for this detailed in the GeoJSON spec. So we should be free to add additional behavior as needed, as long as it is documented.

Can you explain more what you mean by the "next" key? In JavaScript, object properties do not have a guaranteed order.

4.3.3 Object
An object is a member of the type Object. It is an unordered collection of properties each of which contains a primitive value, object, or function. A function stored in a property of an object is called a method.

@ggetz ggetz added the needs feedback On hold until additional info is supplied label Nov 20, 2023
@mfarmer-ara
Copy link
Author

Can you explain more what you mean by the "next" key? In JavaScript, object properties do not have a guaranteed order.

It seems like the order Cesium will loop through the keys in that linked function is dependent on the original order it is in the file (observation not guaranteed).

So, while it doesn't really matter what order the keys are in usually, since it appears their order is kept based on the original file, that function will return what it thinks is the best name based on the order they're lopped through.

Though now I'm confusing myself with the precedence that's in that function.

I may play around with this today/tomorrow to see if I can pin things down.

@mfarmer-ara
Copy link
Author

mfarmer-ara commented Nov 20, 2023

@ggetz I'm still poking at this, but it looks like my example files might have been too simple. I am not able to make it happen with my simpler files but a geojson file with a polygon with 212 points (x,y,z) is consistently saying name (properties.title) is defined and skipping the extra property check. Since I am not sure I can share that one, I made a crazy shape with geojson.io with a similar number of points.

My tests were bad but in the sort of I was tricking myself way. I've figured out a way around this and will put a PR up that will hopefully work for eveyr


Keeping below, but I had an error in my testing files where title was set to be title: and not title for the simple files. So this is only relevant for the randomShape file, whose behavior is what I'm seeing.

Dragging and dropping this with the viewerDragDropMixin then clicking the shape shows the issue. It may be a race condition somewhere and not what I originally thought it might be. A smaller file (those shared earlier) the name is undefined but this file the name is defined as empty.

-- putting console.log({ name, defined: defined(name), properties }); in GeoJsonDataSource.js ~ line 136.

randomShape.json

{ "name": "", "defined": true, "properties": { "title": "", "description": "", "country-name": "North Carolina", "two-letter-code": "NC", "fill": "#f0a3FF", "fill-opacity": "0.5" } }

Files shared earlier (simple shape)
{ "defined": false, "properties": { "title:": "", "description": "", "country-name": "North Carolina", "two-letter-code": "NC", "fill": "#f0a3FF", "fill-opacity": "0.5" } }

simpleShape
randomShape

@mfarmer-ara
Copy link
Author

Sorry for the extra comments while I explored this issue. The fix I've looked is to simply check if title is empty next to the defined() check for GeoJSON data sources.

With only title (no name or includes name/title keys) present and title equal to empty string, the below behavior happens. Is that fine? It behaves just as if title didn't exist at all.

emptyTitleBehavior

@ggetz
Copy link
Contributor

ggetz commented Nov 22, 2023

With only title (no name or includes name/title keys) present and title equal to empty string, the below behavior happens. Is that fine? It behaves just as if title didn't exist at all.

This seems in line with expect behavior, yes.

@ggetz ggetz added type - enhancement and removed needs feedback On hold until additional info is supplied labels Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants