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

FBXLoader: support for colors specified as single floats. #12648

Merged
merged 4 commits into from
Nov 15, 2017

Conversation

looeee
Copy link
Collaborator

@looeee looeee commented Nov 14, 2017

Fix for #12067 (comment)

Extends the parseColor function to support color type Color (rather than ColorRGB), where the value is a single float rather than an RGB triple.

Also fix an error when parsing light color.

@looeee
Copy link
Collaborator Author

looeee commented Nov 14, 2017

@MarcRibguth the error message you were getting was because the light in your model was failing to load. This PR fixes this and sets the light color correctly.

However this doesn't fix the issue that colors of the model in your fan_with color.fbx are not correct - they all load as grey in three.js, whereas the blades and body are red and orange respectively in other loaders. Would you mind opening a seperate issue for that?

@MarcRibguth
Copy link

@looeee thanks for the reply, I will do that.

@mrdoob
Copy link
Owner

mrdoob commented Nov 14, 2017

How about...

function parseColor( property ) {

    var color = new THREE.Color();

    if ( property.type === 'Color' ) {

        return color.setScalar( property.value );

    } else {

        return color.fromArray( property.value );

    }

}

@looeee
Copy link
Collaborator Author

looeee commented Nov 15, 2017

@mrdoob done.

BTW, which style do you prefer in the library?

1

function parseColor( property ) {

    var color = new THREE.Color();

    if ( property.type === 'Color' ) {

        return color.setScalar( property.value );

    } else {

        return color.fromArray( property.value );

    }

}

2

function parseColor( property ) {

    var color = new THREE.Color();

    if ( property.type === 'Color' ) {

        return color.setScalar( property.value );

   }

    return color.fromArray( property.value );

}

@mrdoob
Copy link
Owner

mrdoob commented Nov 15, 2017

Hmm, actually, yeah... In this case 2. is prettier 😇

@looeee
Copy link
Collaborator Author

looeee commented Nov 15, 2017

OK, updated 🙂

@mrdoob
Copy link
Owner

mrdoob commented Nov 15, 2017

Another option would be:

function parseColor( property ) {

    var color = new THREE.Color();

    switch ( property.type ) {
        case 'ColorRGB':
            return color.fromArray( property.value );
        case 'Color':
            return color.setScalar( property.value );
    }

}

It depends of how many other types there are.

@mrdoob
Copy link
Owner

mrdoob commented Nov 15, 2017

Merging for now.

@mrdoob mrdoob merged commit a8f1818 into mrdoob:dev Nov 15, 2017
@mrdoob
Copy link
Owner

mrdoob commented Nov 15, 2017

Thanks!

@looeee
Copy link
Collaborator Author

looeee commented Nov 16, 2017

As far as I'm aware it should just be either 'Color' (float) or 'ColorRGB' (array).

In any case, if there are more types used (e.g. 'Number', 'Double', 'Vector3' etc ) then they will all map to either an array or float, so I would favour

function parseColor( property ) {

    var color = new THREE.Color();

    if( Array.isArray( propery.value ) { 

        return color.fromArray( property.value );

    }

    return color.setScalar( property.value );

}

However I think the current function is sufficient.

@looeee looeee deleted the FBXLoader_parseColor_fix branch January 27, 2022 10:24
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