Do not throw when there are less unique positions than required for rendering. #2386
Conversation
…polyline and polygon geometries return undefined when there are not enough unique points to create the geometry rather than throwing an exception.
| @@ -812,6 +820,11 @@ define([ | |||
| this._state = PrimitiveState.COMBINING; | |||
|
|
|||
| when(promise, function(packedResult) { | |||
| if (!defined(packedResult)) { | |||
| that._state = PrimitiveState.FAILED; | |||
There was a problem hiding this comment.
What triggers this if block? If zero geometry is created because all instances were invalid after deduplication?
There was a problem hiding this comment.
Everything else looks fine; I just want to better understand what the change in behavior is. If I create a PolylineGeometry with 3 identical points, the Primitive will no longer throw, but it looks like the state becomes failed? Does that also mean Primitive.ready will never be true either?
There was a problem hiding this comment.
One more question, if I create a batch with 3 per-color instance geometries and one of them fails. What happens when I call getGeometryInstanceAttributes does that still work?
Ideally (in my opinion) for all intents and purposes duplicate geometries would lead to that particular geometry not being drawn, but everything else would be the same.
There was a problem hiding this comment.
What triggers this if block? If zero geometry is created because all instances were invalid after deduplication?
Yes.
Does that also mean Primitive.ready will never be true either?
Yes, I'll change that.
What happens when I call getGeometryInstanceAttributes does that still work?
If one of them fails, then it will return an empty object. If all of them are invalid, it will return undefined. I think it should be one or the other so I'll make it consistent. You think it should return a valid object to get and set the attribute, but not actually do anything? I think that might be useful so users won't have to check for undefined, but it'll be slower when creating new types arrays and it'll waste memory.
There was a problem hiding this comment.
I think that might be useful so users won't have to check for undefined, but it'll be slower when creating new types arrays and it'll waste memory.
Since most of the time, geometry is valid, I think it's perfectly fine to always return an object (since when it's valid there's no slow down). The rare occasion that the geometry didn't get created isn't worth the pain of having users have to check for undefined the rest of the time. For example, all of the entity batching code for geometry assumes that a valid object will be returned if attributes were set; so I would have to update that if it suddenly starts returning undefined. If we expected lots of failures to be the norm, then I would feel differently.
|
Update CHANGES.md. |
…stance with invalid geometry.
…hen batched asynchronously.
Conflicts: CHANGES.md
|
@mramato This is ready for another look. I updated CHANGES.md, but I wasn't sure if this should go in the |
|
This all looks good to me, I wouldn't count this as a breaking change; more like an improvement in expected behavior. @pjcozzi did you want to look at this again? If not, feel free to hit merge. |
|
I agree with @mramato about the breaking change. |
Do not throw when there are less unique positions than required for rendering.
For #2375.
Geometry.createGeometrycan now returnundefined. If it does returnundefined, it adds nothing to the batched geometry/draws nothing.