Fix StaticGeometryMaterialBatch remove #7163
Conversation
|
Thanks for the pull request @hpinkos!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
Thanks @hpinkos! Since we have some existing unit tests that are simply not checking the return value, please update them to do so, so this doesn't happen again. |
|
@mramato Which unit tests are you referring to? I didn't see any that are using the |
|
I thought the various specs were calling remove already, but maybe not. Either way it would be good to add something to StaticGeometryPerMaterialBatchSpecs and other similar specs. Entity show issues keep regressing over and over again, so we need to step up our game in this area. |
|
Yeah I should be able to add a unit test of the material batch problem pretty easily. All of these static batch files could use a major refactor. I had to stop myself from making more changes. They could really benefit from sharing a common parent class. |
|
Yep, all for refactoring things down the line when we have time. Right now we just need to stop the bleeding. |
|
@mramato ready |
|
You have an eslint failure |
|
whoops, thanks @mramato! This is ready |
| expect(scene.primitives.length).toEqual(2); | ||
| }) | ||
| .then(function() { | ||
| batch.remove(updater1); |
There was a problem hiding this comment.
When we have time to refactor these things, we should probably have remove return true/false at this level as well so we can test that items are added/removed as expected.
Fixes #7160
StaticGeometryMaterialBatchBatch.removewas returning the wrong thing, causingStaticGeometryMaterialBatchto think the updater was removed from a batch it wasn't removed from.Also cleaned up remove for
StaticGeometryColorBatchandStaticOutlineGeometryBatchwhich weren't returning anything from the remove functions.