Skip to content
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

InstancedMesh: Honor instanceColor in toJSON() and ObjectLoader. #21486

Merged
merged 2 commits into from
Mar 25, 2021

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.

src/core/Object3D.js Outdated Show resolved Hide resolved
@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