Allow for more tolerant processing of KML

[mmacaula]

This PR addresses an issue we came across when implementing KML support for our customer.

The KML we had to ingest incorrectly contained some placemarks that looked like:

<Placemark>
      <MultiGeometry>
            <LineString id=" - Full Path">
                  <extrude>1</extrude>
                  <altitudeMode>clampToGround</altitudeMode>
                  <tessellate>1</tessellate>
                  <coordinates>-116.166667,36,0 -116.166667,35,0 </coordinates>
            </LineString>
            <Point id="">
                  <altitudeMode>clampToGround</altitudeMode>
                  <coordinates>-116.166667,35.5,0 </coordinates>
            </Point>
      </MultiGeometry>
      <styleUrl>#defaultstyle</styleUrl>
</Placemark>
<Placemark>
      <name />
      <MultiGeometry>
            <LineString id=" - Full Path">
                  <extrude>1</extrude>
                  <altitudeMode>clampToGround</altitudeMode>
                  <tessellate>1</tessellate>
                  <coordinates>-116.083333,36,0 -116.083333,35,0 </coordinates>
            </LineString>
            <Point id="">
                  <altitudeMode>clampToGround</altitudeMode>
                  <coordinates>-116.083333,35.5,0 </coordinates>
            </Point>
      </MultiGeometry>
      <styleUrl>#defaultstyle</styleUrl>
</Placemark>

The problem was that this KML was reusing ids inside of placemarks. The empty string id="" and the id=" - Full Path".

This is definitely malformed KML, and doesn't pass validators, however, it does work in Google Earth. This PR makes changes that provides a context-id for parsing placemarks that essentially prepends the placemark entity id to the id of any entity in the multi-geometry. I also check for an empty string in the getOrCreateEntity method to handle that case.

I can create an issue if that fits your workflow better.

[pjcozzi]

Wow, two pull requests in one day, thanks again @mmacaula!

@tfili can you review and merge when ready?

[tfili]

Use single quotes for strings. Here you may just want to use id.length !== 0 since you are just checking for a non-empty string.

[mmacaula]

@tfili Done and fixed merge conflicts

[hpinkos]

Does this fix #3941?

[mmacaula]

@hpinkos, no. This uses placemarks as the context for items underneath so that wouldn't fix that issue.

I'm not 100% sure how to solve that issue while still allowing for network links to update the same entities by id correctly. For example, if I have a network link that updates the position of a placemark named 'foo' and an id of 'foo' and that network link refreshes, then obviously I'd want the foo placemark to move, not have two foo placemarks.

Perhaps the proper 'context' for using ignoring duplicate ids is on refreshable network links? It's not perfect but not having any insight into what GE is doing, it might be a good balance of trying to be true to spec and handling this crappy KML.

[hpinkos]

Okay cool, thanks for the info! It sounded similar to that issue so I wanted to check.

[denverpierce]

GE doesn't seem to particularly care about the ID's. I tested a network link KML with a lot of duplicate ID's, editing and refreshing, and and it treated them all as if they were unique no matter what the ID was.

[tfili]

@mmacaula Can you resolve the conflicts again? Sorry, i was out most of last week. I'll test as soon as its updated.

[mramato]

GE doesn't seem to particularly care about the ID's. I tested a network link KML with a lot of duplicate ID's, editing and refreshing, and and it treated them all as if they were unique no matter what the ID was

GE absolutely cares about the ids if you are working with NetworkLinkControl. If the ids are dupes or incorrect, the file won't work. Most NetworkLinks are full refreshes and not updates, and ids won't matter in those cases.

[denverpierce]

You are correct, my mistake. Loading a networklinkcontrol and using targetID on a file with duplicate ID's throws an 'id not found' and fails to parse the networklinkcontrol file.

[mmacaula]

@tfili done with merge (perhaps updating CHANGES.md is something that should be done right before final merge?)

It seems like the best way to match GE (and therefore provide best experience for Cesium KML users) is to always create a new entity except when inside an updating network link? This would mean not using the same id as the kml node for the entity id, except when loading a refreshing network link.

Then this would fix #3941, a nice bonus...

Thoughts?

[tfili]

This looks good to me. @mramato Do you have any comments before I merge this?

[mramato]

Thanks @mmacaula, looks good to me.