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

TGA files support for FBXLoader #13841

Merged
merged 3 commits into from
Apr 14, 2018
Merged

TGA files support for FBXLoader #13841

merged 3 commits into from
Apr 14, 2018

Conversation

NicekDev
Copy link
Contributor

@NicekDev NicekDev commented Apr 12, 2018

Based on @looeee #13227 PR and discussed in #13786
It is without DDS support that was causing problems.

I know line 369 is ugly and maybe not best way to do it, could you advice me better way?
if (textureNode.FileName.slice(textureNode.FileName.lastIndexOf('.') + 1)=='tga')

previous if ( fileName.indexOf( 'image/tga' ) !== - 1 ) is not working in this version...

Based on @looeee  mrdoob#13227 PR and discussed in  mrdoob#13786
It is without DDS support that was causing problems.

I know line 369 is ugly and maybe not best way to do it, could you advice me better way?
`if (textureNode.FileName.slice(textureNode.FileName.lastIndexOf('.') + 1)=='tga')`
@NicekDev NicekDev mentioned this pull request Apr 12, 2018
12 tasks

var texture;

if (textureNode.FileName.slice(textureNode.FileName.lastIndexOf('.') + 1)=='tga'){
Copy link
Collaborator

@takahirox takahirox Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need spaces to follow the code style. And recommended to use === instead of ==.

if ( textureNode.FileName.slice( textureNode.FileName.lastIndexOf( '.' ) + 1 ) === 'tga' ) {

Copy link
Contributor Author

@NicekDev NicekDev Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @takahirox,

thanks for suggestions. I updated the PR style. Anyway I still don't know if it is the right way to check if this is TGA file. It works but maybe there is better way?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK, but you need to account for capitalised file extensions like "TGA".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about if ( textureNode.FileName.slice( -3 ).toLowerCase() === 'tga' ) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right @looeee. I updated the PR.

Copy link
Collaborator

@takahirox takahirox Apr 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO using .lastIndexOf( '.' ) would be more robust. For example, if ( textureNode.FileName.slice( -3 ).toLowerCase() === 'tga' ) can be match with like 'something.atga'.

I haven't looked into FBXLoader recently. Is textureNode.FileName guaranteed to be one of the known image extensions here? If so, .slice( -3 ) could be ok.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't find jpeg though...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had an easier solution textureNode.FileName.slice( -4 ).toLowerCase() === '.tga', anyways.

updated following #takahirox suggestions
@mrdoob mrdoob requested a review from looeee April 12, 2018 23:34
@mrdoob mrdoob added this to the r92 milestone Apr 12, 2018
@mrdoob mrdoob merged commit 25446d6 into mrdoob:dev Apr 14, 2018
@mrdoob
Copy link
Owner

mrdoob commented Apr 14, 2018

Thanks!

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.

None yet

4 participants