-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Comments
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. |
I wonder if previously, the 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. |
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 |
Scott helped out, I may have a PR soon. The order here needs to be corrected, and additionally, Entity's |
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 theposition
data from one side of the merge and clamps it to theavailability
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
The text was updated successfully, but these errors were encountered: