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

Remove JSONLoader from core #15310

Merged
merged 1 commit into from
Nov 26, 2018
Merged

Remove JSONLoader from core #15310

merged 1 commit into from
Nov 26, 2018

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Nov 23, 2018

This PR does two things:

  • It removes JSONLoader from core. The loader is now located in examples/js/loaders/deprecated/JSONLoader.js.
  • It removes ObjectLoader's dependency to JSONLoader. This mean ObjectLoader is not able to load geometries of type Geometry anymore. Instead, an error is logged.

@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2018

Lets see how many people bump into this...

@mrdoob mrdoob merged commit e1ebf32 into mrdoob:dev Nov 26, 2018
@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2018

Thanks!

@RemusMar
Copy link
Contributor

@Mugen87 : "This mean ObjectLoader is not able to load geometries of type Geometry anymore."

This was a bad move.

p.s.
You didn't get the usual 👍 from @looeee
Is he on vacation?

@mrdoob
Copy link
Owner

mrdoob commented Nov 26, 2018

@Mugen87 : "This mean ObjectLoader is not able to load geometries of type Geometry anymore."

This was a bad move.

@Mugen87 how about doing something like this:

if ( "THREE" in window && "JSONLoader" in THREE ) {

// old code goes here

}

So at least we can offer people this solution:

<script src="../build/three.js" /></script>
<script src="js/loaders/deprecated/JSONLoader.js"></script>

You didn't get the usual 👍 from @looeee
Is he on vacation?

He is busy writing a fantastic book 😍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 27, 2018

I'm not sure about this. In general, I would prefer a hard cut and "force" people to use the better and future-proof geometry format. If we start introducing such fallbacks, we also have to do this for SkinnedMesh and all other places in the library with a dependency to Geometry.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 27, 2018

Here is some code users might want to use to convert Geometry to BufferGeometry for a hierarchy of Object3Ds. After the call of convertToBufferGeometry(), it should be possible to call object.toJSON() and replace the contents of the old JSON file.

const map = new Map();

function convertToBufferGeometry( object ) {

	object.traverse( ( child ) => {
	
		const geometry = child.geometry;
	
		if ( geometry && geometry.isGeometry ) {
		
			const uuid = geometry.uuid;
			let bufferGeometry;
		
			if ( map.has( uuid ) ) {
			
				bufferGeometry = map.get( uuid );
			
			} else {
			
				bufferGeometry = new THREE.BufferGeometry().fromGeometry( geometry );
				map.set( uuid, bufferGeometry );
			
			}
		
			child.geometry = bufferGeometry;
		
		}
		
	} );

}

@looeee
Copy link
Collaborator

looeee commented Nov 27, 2018

I'm not sure about this. In general, I would prefer a hard cut and "force" people to use the better and future-proof geometry format.

How about giving people some notice before forcing them to make the change?

if ( "THREE" in window && "JSONLoader" in THREE ) {

    // old code goes here

    console.warn( 'After R105 the ObjectLoader will no longer be able to load geometries of type Geometry. Please update your code before then.' ); 

}

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 27, 2018

Then you also have to do this for SkinnedMesh, see #15314. Geometry support was removed without an announcement. You would have to add a log now and remove the support in six releases. This would be just consequent.

TBH, I vote against this. It is not hard to convert (at least static) models which rely on Geometry to BufferGeometry. I have used the same approach posted here for #15297.

A more determined strategy will allow us to finally move forward with stuff like #11603

@mrdoob
Copy link
Owner

mrdoob commented Nov 27, 2018

If we start introducing such fallbacks, we also have to do this for SkinnedMesh and all other places in the library with a dependency to Geometry.

The difference with SkinnedMesh is that the user can add code to their project and things would still work. See 803bb9f 6e89f67

TBH, I vote against this. It is not hard to convert (at least static) models which rely on Geometry to BufferGeometry. I have used the same approach posted here for #15297.

In order to run that script they'll have to load the file first. Now they have no way of doing that unless they replace the whole ObjectLoader.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 28, 2018

In order to run that script they'll have to load the file first. Now they have no way of doing that unless they replace the whole ObjectLoader.

Yes. You would have to do the conversion before you upgrade the library.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Nov 28, 2018

But well, I'm feeling you prefer the fallback option. I'll add a PR 😇

@titansoftime
Copy link
Contributor

This really sucks. I now have to COMPLETELY redo my workflow. Don't like being forced to do this.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 3, 2018

I know it sucks but we have to move away from Geometry. It's just a matter of time until WebGLRenderer will stop rendering Geometry.

Don't like being forced to do this.

No one forces you to upgrade the library 😇

@abdulsattar
Copy link

Hi, can you please explain the reason why Geometry loader is being deprecated?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 29, 2018

From a performance point of view Geometry is an inefficient representation of geometric data. It creates for each primitive like a vertex or a normal vector separate objects. Object creation is quite expensive in JavaScript so it's better to represent the geometry in form of array buffer data. This is actually the format you need in order to upload the data to the GPU via WebGL. Because of this, three.js internally performs the conversion to BufferGeometry when using Geometry which also produces unnecessary overhead.

So the loader was removed since the respective format is going to be deprecated. More information right here #15387

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.

6 participants