Update Model.js #9197
Update Model.js #9197
Conversation
Fix for issue CesiumGS#9169
|
Thank you so much for the pull request @shelterit! I noticed this is your first pull request and I wanted to say welcome to the Cesium community! The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.
Reviewers, don't forget to make sure that:
|
|
Thanks for opening a PR Alex! I know you mentioned testing for unintended consequences, but I'm hesitant to merge when we don't have a way to verify this fix. It may not cause any adverse effects but it may push the error further down the line since the underlying cause is not fixed. Why is an undefined command being pushed here in the first place? Do you know where it originates? |
|
Hey,
That's a good question, however I don't have the internal knowledge of
Cesium to answer how that could happen. :) I'll see if I can talk to the
customer if they can share that piece of data with us, although I'm not
sure (they're a bit particular, hush, hush). I can probably get the source
files, and find out what they used to tile it, maybe there's a race
condition in there somewhere, and hopefully at the least try to make a
different model to replicate the issue.
If I go through the original file, what should I be looking for? It's all
linked to whether a property is present in the right place, and possibly
the tiler / somewhere didn't inject the property at some place? Again, I'm
not smart enough to know what goes into the opacity command being sent and
what that stack looks like.
We've currently got this fix in our build stream, but it would be great to
get it out, of course, to keep things "pure". Do you have a test suite of
data for every build that you could test the 'fix' against?
Cheers,
Alex
…On Mon, Nov 2, 2020 at 4:11 AM Omar Shehata ***@***.***> wrote:
Thanks for opening a PR Alex! I know you mentioned testing for unintended
consequences, but I'm hesitant to merge when we don't have a way to verify
this fix. It may not cause any adverse effects but it may push the error
further down the line since the underlying cause is not fixed. Why is an
undefined command being pushed here in the first place? Do you know where
it originates?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9197 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFHK5XFH55QDRXJR4EN4O3SNWJDRANCNFSM4SNQGIYA>
.
--
Information Alchemist / UX consultant / GUI developer for hire
http://thinkplot.org | http://www.linkedin.com/in/shelterit
|
|
Thanks again for your contribution @shelterit! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
2 similar comments
|
Thanks again for your contribution @shelterit! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
Thanks again for your contribution @shelterit! No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy? I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with |
|
A similar fix was made in #9271 so I'm going to close this one. Thanks @shelterit. |
Fix for issue #9169
Added a simple test for those cases when commands are dynamic in a model