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

Merged Entity availability #7717

Closed
emackey opened this issue Apr 5, 2019 · 4 comments · Fixed by #7718
Closed

Merged Entity availability #7717

emackey opened this issue Apr 5, 2019 · 4 comments · Fixed by #7718

Comments

@emackey
Copy link
Contributor

emackey commented Apr 5, 2019

Here's some code from where two Entities are merged into one:

https://github.com/AnalyticalGraphicsInc/cesium/blob/502f34abebb4ffb06dfc1671db0983c52fdffe70/Source/DataSources/Entity.js#L568-L569

The order of parameters on the merged name property is the same order as all of the other merged properties in this merge function.

But availability is backwards. It actually prefers the lower-priority Entity over the higher-priority one, when all the other properties go the other way.

This has some strange consequences for entity.path for example, as it takes the position data from one side of the merge and clamps it to the availability from the other side of the merge. As a quick test, flipping these two parameters on line 569 here fixed the problem in an app with a CompositeEntityCollection made from different time intervals. But is that the right answer in all cases? Why were they placed backwards here?

/cc @shunter @mramato

@mramato
Copy link
Contributor

mramato commented Apr 5, 2019

I remember this being intentional but I have absolutely no recollection as to why. Git blame wasn't any help.

I'll let my brain work on it and see if I can remember, otherwise I'm not against changing it as long as we document what the expect behavior is and why this time.

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2019

I wonder if previously, the position and availability always came from the base layer, and the upper layers were just there to superimpose some different graphics properties. I could imagine the upper layer availability being a subset of the base layer in such a case, although in my app I have the opposite happening.

Perhaps the merged entity availability should actually be a union of the two availabilities, unless one of them is infinite, in which case it should use the other. That sounds complex and prone to error though.

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2019

OK, I have a confirmed race condition now. If the entity becomes available in one data source before it becomes available in the other one, that controls the identity of which is this and which is source, not the order of collections in the composite collection, as one would expect.

@emackey
Copy link
Contributor Author

emackey commented Apr 5, 2019

Scott helped out, I may have a PR soon. The order here needs to be corrected, and additionally, Entity's clean helper was not cleaning out name or availability, making those two properties merge in order of first collection to finish loading, instead of being recomposited in order of the composite collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants