-
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
Update Primitive and GroundPrimitive to use BatchTable #4361
Conversation
…cking/unpacking pick ids for transfer to/from web workers.
var length = instances.length; | ||
|
||
var attributesInAllInstances = []; | ||
var attributes0 = instances[0].attributes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
length
will always be >= 1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see this is old code.
I have 6 failures in master:
And 10 failures in this branch:
|
@pjcozzi This is ready for another look. |
@@ -59,6 +59,9 @@ define([ | |||
|
|||
if (toWorld) { | |||
for (i = 0; i < length; ++i) { | |||
if (!defined(instances[i].geometry)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but why use continue
here? Why not, just if (defined(...
or even put the check in transformToWorldCoordinates
since the semantics would still be fine just like remove(undefined)
is OK with our collections.
@@ -214,18 +136,14 @@ define([ | |||
// Clip to IDL | |||
if (!scene3DOnly) { | |||
for (i = 0; i < length; ++i) { | |||
if (!defined(instances[i].geometry)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment.
|
||
var i = 1; | ||
while (i < buffer.length) { | ||
if(buffer[i++] === 1.0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whitespace
Just a few trivial comments. All tests pass. Are you sure coverage for the new/moved code is solid? |
@pjcozzi This is ready for another look.
Yes. After this change I had to go through most of the tests because they were failing. I also ran through all of the Sandcastle examples. Now that VTF is required, should we throw a There shouldn't be any problems with the current built-in per-instance attributes since they are all unsigned bytes, but if a user creates a custom appearance and needs floating point per-instance attribute a |
Not in |
@pjcozzi I added the exceptions. This is ready for another look. |
Updates
Primitive
andGroundPrimitive
to useBatchTable
instead of per-instance attributes.This needs to be merged before #4309. It will fix the
PolylineGeometryUpdater
crashes in that branch.