Skip to content

Conversation

@Michael4d45
Copy link
Contributor

Description

I was having troubles transferring InstancedMesh data due to the Object3D.toJSON and ObjectLoader.parseObject functions not accounting for the instanceColor attribute.

I've added instanceColor to the InstancedMesh cases in the respective functions.
Since instanceColor is optional, I've mimicked the behavior in InstanceMesh.copy() to check if it's null.

@Mugen87 Mugen87 added this to the r127 milestone Mar 20, 2021
@Mugen87 Mugen87 changed the title Keep instanceColor for InstancedMesh in toJSON and ObjectLoader InstancedMesh: Honor instanceColor in toJSON() and ObjectLoader. Mar 20, 2021
@Mugen87 Mugen87 merged commit 46c947c into mrdoob:dev Mar 25, 2021
@mrdoob
Copy link
Owner

mrdoob commented Mar 25, 2021

I was not sure about this one yet.
What if we wanted to add support for alpha? 🤔

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 25, 2021

Since setColorAt() already assumes three component colors, I think have this issue independently of this PR.

this.instanceColor = new BufferAttribute( new Float32Array( this.count * 3 ), 3 );

Maybe we always assume RGBA when vertex color alpha is supported?

We could change the signature of setColorAt() so it supports a third alpha parameter or introduce setColorAlphaAt().

@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2021

I guess what I meant is that we should do this to be safe:

if ( instanceColor !== undefined ) object.instanceColor = new BufferAttribute( new Float32Array( instanceColor.array ), instanceColor.itemSize );

@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2021

2f1fa4e

@mrdoob
Copy link
Owner

mrdoob commented Mar 27, 2021

Hmm... Maybe we could introduce a fromJSON() static method to BufferAttribute?

if ( instanceColor !== undefined ) object.instanceColor = BufferAttribute.fromJSON( instanceColor );

I guess such change would then apply to maaany other classes 😬

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 28, 2021

Related #11266.

It's definitely worth to investigate this idea. I just wonder if fromJSON() should always return BufferAttribute or the more concrete types e.g. Float32BufferAttribute or Uint16BufferAttribute? I guess it does not matter a lot but there could be the case that somebody relies on using these classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants