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

fixing merge() function not accepting with BufferGeometry-typed argument #21207

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@salaros

salaros commented Nov 2, 2017

Example of the error I was getting before applying this fix:

TS2345: Argument of type 'Geometry | BufferGeometry' is not assignable to parameter of type 'Geometry'

How to reproduce:

import { ObjectLoader } from 'three';

let objectInJson = <get the JSON object>;
let loader = new THREE.ObjectLoader();
let object = loader.parse( objectInJson  );
let geometry = new Geometry();
object.traverse( function(child) {
    if(child instanceof Mesh) {
        geometry.merge( child.geometry );
    }
});
fixing merge function not working with BufferGeometry
TS2345: Argument of type 'Geometry | BufferGeometry' is not assignable to parameter of type 'Geometry'.
@dt-bot

This comment has been minimized.

Show comment
Hide comment
@dt-bot

dt-bot Nov 2, 2017

Member

types/three/three-core.d.ts

to authors (@gyohk @florentpoujol @SereznoKot @omni360 @ivoisbelongtous @piranha771 @qszhusightp @nakakura @s093294 @Pro @efokschaner Kon (account can't be detected)). Could you review this PR?
👍 or 👎?

Member

dt-bot commented Nov 2, 2017

types/three/three-core.d.ts

to authors (@gyohk @florentpoujol @SereznoKot @omni360 @ivoisbelongtous @piranha771 @qszhusightp @nakakura @s093294 @Pro @efokschaner Kon (account can't be detected)). Could you review this PR?
👍 or 👎?

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Nov 2, 2017

Member

@salaros Please fix the failures indicated in the Travis CI log.

Member

RyanCavanaugh commented Nov 2, 2017

@salaros Please fix the failures indicated in the Travis CI log.

@salaros

This comment has been minimized.

Show comment
Hide comment
@salaros

salaros Nov 2, 2017

@RyanCavanaugh I don't know if you have already read the log, but the failures reported by Travis-CI has nothing to do with my fix! The errors reported by Travis are tests-related (ban-comma-operator).
Please see a small portion of log below.

Do you want me to create yet another PR with fixes for this errors? Because IMO ban-comma-operator error has to be fixed apart. It's not a good practice to mix fixes for the main code-base with test-related stuff (I mean when tests failing are not directly affected by the changes committed to the code), I hope you understand what I mean.

Please accept this PR and I will create another one which fixes tests.

Error in three

Command failed: node /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js

Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/css3d/css3d_sprites.ts

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 141:57 ban-comma-operator Don't use the comma operator.

/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/webgl/webgl_materials.ts

ERROR: 181:62 ban-comma-operator Don't use the comma operator.

at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:76:19

at Generator.next (<anonymous>)

at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:5:58)

at <anonymous>

Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/css3d/css3d_sprites.ts

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 141:57 ban-comma-operator Don't use the comma operator.

/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/webgl/webgl_materials.ts

ERROR: 181:62 ban-comma-operator Don't use the comma operator.

at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:76:19

at Generator.next (<anonymous>)

at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:5:58)

at <anonymous>

salaros commented Nov 2, 2017

@RyanCavanaugh I don't know if you have already read the log, but the failures reported by Travis-CI has nothing to do with my fix! The errors reported by Travis are tests-related (ban-comma-operator).
Please see a small portion of log below.

Do you want me to create yet another PR with fixes for this errors? Because IMO ban-comma-operator error has to be fixed apart. It's not a good practice to mix fixes for the main code-base with test-related stuff (I mean when tests failing are not directly affected by the changes committed to the code), I hope you understand what I mean.

Please accept this PR and I will create another one which fixes tests.

Error in three

Command failed: node /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js

Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/css3d/css3d_sprites.ts

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 141:57 ban-comma-operator Don't use the comma operator.

/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/webgl/webgl_materials.ts

ERROR: 181:62 ban-comma-operator Don't use the comma operator.

at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:76:19

at Generator.next (<anonymous>)

at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:5:58)

at <anonymous>

Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/css3d/css3d_sprites.ts

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 32:17 ban-comma-operator Don't use the comma operator.

ERROR: 141:57 ban-comma-operator Don't use the comma operator.

/home/travis/build/DefinitelyTyped/DefinitelyTyped/types/three/test/webgl/webgl_materials.ts

ERROR: 181:62 ban-comma-operator Don't use the comma operator.

at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:76:19

at Generator.next (<anonymous>)

at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:5:58)

at <anonymous>
@efokschaner

This comment has been minimized.

Show comment
Hide comment
@efokschaner

efokschaner Nov 3, 2017

Contributor

These lint issues also show up in #21171 and #21192

Having a look now to see why it might have happened.

On the plus side, they should be solved if you wait for #21171 to be merged and then pull it in to your branch.

I'll follow up shortly with a review of your changes.

Contributor

efokschaner commented Nov 3, 2017

These lint issues also show up in #21171 and #21192

Having a look now to see why it might have happened.

On the plus side, they should be solved if you wait for #21171 to be merged and then pull it in to your branch.

I'll follow up shortly with a review of your changes.

@efokschaner

This comment has been minimized.

Show comment
Hide comment
@efokschaner

efokschaner Nov 3, 2017

Contributor

Hey @salaros , judging from the implementation, you cannot pass a BufferGeometry to merge, see https://github.com/mrdoob/three.js/blob/master/src/core/Geometry.js#L704 , BufferGeometry does not seem to have the isGeometry tag.

Does that seem right to you?

Contributor

efokschaner commented Nov 3, 2017

Hey @salaros , judging from the implementation, you cannot pass a BufferGeometry to merge, see https://github.com/mrdoob/three.js/blob/master/src/core/Geometry.js#L704 , BufferGeometry does not seem to have the isGeometry tag.

Does that seem right to you?

@salaros

This comment has been minimized.

Show comment
Hide comment
@salaros

salaros Nov 3, 2017

So might this definition be a problem?

I mean if BufferGeometry type doesn't have isGeometry could we really use it as an alternative to Geometry type on Mesh/Object3D.geometry property?

export class Mesh extends Object3D {
    constructor(geometry?: Geometry | BufferGeometry, material?: Material | Material []);

    geometry: Geometry|BufferGeometry;
 
   .......
}

I feel that in vanilla Three.js or @types/three there is some sort of type inconsistency across Geometry-related methods and properties.
For now I will use an extra type check in my code snippet to avoid compilation errors:

this.object.traverse( function(child) {
    if(child instanceof Mesh && child.geometry instanceof Geometry) {
         geometry.merge( child.geometry );
     }
});

However if you think my there is still something wrong with TS wrapper, just let me know - I really willing to help.

salaros commented Nov 3, 2017

So might this definition be a problem?

I mean if BufferGeometry type doesn't have isGeometry could we really use it as an alternative to Geometry type on Mesh/Object3D.geometry property?

export class Mesh extends Object3D {
    constructor(geometry?: Geometry | BufferGeometry, material?: Material | Material []);

    geometry: Geometry|BufferGeometry;
 
   .......
}

I feel that in vanilla Three.js or @types/three there is some sort of type inconsistency across Geometry-related methods and properties.
For now I will use an extra type check in my code snippet to avoid compilation errors:

this.object.traverse( function(child) {
    if(child instanceof Mesh && child.geometry instanceof Geometry) {
         geometry.merge( child.geometry );
     }
});

However if you think my there is still something wrong with TS wrapper, just let me know - I really willing to help.

@efokschaner

This comment has been minimized.

Show comment
Hide comment
@efokschaner

efokschaner Nov 3, 2017

Contributor

I don't think the Mesh definition is wrong, see: https://threejs.org/docs/#api/objects/Mesh.geometry

Instead of excluding BufferGeometry you could also consider handling them, something like this (untested):

this.object.traverse( function(child) {
    if(child instanceof Mesh) {
        if(child.geometry instanceof Geometry){
            geometry.merge( child.geometry );
        } else {
            let convertedGeometry = new Geometry();
            convertedGeometry.fromBufferGeometry(child.geometry);
            geometry.merge( convertedGeometry );
        }
     }
});

I will say that three.js is actually pretty unfriendly to static typing, it relies a lot on javascript's dynamic nature internally and on its interfaces, and so the type definitions here are far from perfect.

I appreciate you taking the time to help, and if you find anything else that could be improved I'd be pleased to hear from you again.

Contributor

efokschaner commented Nov 3, 2017

I don't think the Mesh definition is wrong, see: https://threejs.org/docs/#api/objects/Mesh.geometry

Instead of excluding BufferGeometry you could also consider handling them, something like this (untested):

this.object.traverse( function(child) {
    if(child instanceof Mesh) {
        if(child.geometry instanceof Geometry){
            geometry.merge( child.geometry );
        } else {
            let convertedGeometry = new Geometry();
            convertedGeometry.fromBufferGeometry(child.geometry);
            geometry.merge( convertedGeometry );
        }
     }
});

I will say that three.js is actually pretty unfriendly to static typing, it relies a lot on javascript's dynamic nature internally and on its interfaces, and so the type definitions here are far from perfect.

I appreciate you taking the time to help, and if you find anything else that could be improved I'd be pleased to hear from you again.

@salaros

This comment has been minimized.

Show comment
Hide comment
@salaros

salaros Nov 3, 2017

OK, thanks for the interesting solution, I will definitely give it a try

salaros commented Nov 3, 2017

OK, thanks for the interesting solution, I will definitely give it a try

@salaros salaros closed this Nov 3, 2017

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