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

Updated to THREE r89 #11

Closed
wants to merge 1 commit into from
Closed

Updated to THREE r89 #11

wants to merge 1 commit into from

Conversation

RemusMar
Copy link
Contributor

No description provided.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

@RemusMar SEA3D.js is an SDK. It should have no dependence.

@RemusMar
Copy link
Contributor Author

What dependence?
in r89 they moved "decodeText" and "extractUrlBase" from Loader to LoaderUtils.
That's all.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

In this case, we should extractUrlBase in SEA3D.js

@RemusMar
Copy link
Contributor Author

RemusMar commented Dec 20, 2017

See this:
https://github.com/mrdoob/three.js/blob/dev/src/loaders/LoaderUtils.js

extractUrlBase: function ( url ) {
		var parts = url.split( '/' );
		if ( parts.length === 1 ) return './';
		parts.pop();
		return parts.join( '/' ) + '/';
}

@RemusMar
Copy link
Contributor Author

RemusMar commented Dec 20, 2017

SEA3D.js is an SDK. It should have no dependence.

Not true.
In the previous version it was:

this.config.path = THREE.Loader.prototype.extractUrlBase( url );

So we should keep them both.
Besides, SEA3D.js without THREE.js does not make much sense.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

Not true.
In the previous version it was:

it is a bug.

Besides, SEA3D.js without THREE.js does not make much sense.

So one can use sea3d in another library for example... but the purpose is not this.
Today sea3d is only used for graphics, I am releasing an updates that have extended its functionality using SEA3D.js, SEA3DLZMA.js and others codes to load an entire project including a threejs(lzma-compressed and possible others algorithms) inside .sea file.

I think in sea3d.js should be like this:

if (!this.config.path) {

	this.config.path = this.extractUrlBase( url );

}

@RemusMar
Copy link
Contributor Author

"extractUrlBase" does not exist in the SEA3D files.

@RemusMar
Copy link
Contributor Author

RemusMar commented Dec 20, 2017

If you want to make SEA3D independent of THREE, you need to include both functions from LoaderUtils to SEA3D.
In my opinion, not a good idea because It's wasted resources (memory) for nothing.

@RemusMar
Copy link
Contributor Author

In my opinion: for r89 we should keep them how they are now.
They identified stack overflows for large strings in old browsers, they will come with more fixes,
so It's better to rely on the THREE functions for now.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

"extractUrlBase" does not exist in the SEA3D files.

Yes, my suggests we should add extractUrlBase function inside sea3d.js.

In my opinion, not a good idea because It's wasted resources (memory) for nothing.

I do not think that some lines of code will overload something. My concern would be just with maintaining of code but for this case is necessary that sea3d.js has no dependency.

@RemusMar
Copy link
Contributor Author

Yes, my suggests we should add extractUrlBase function inside sea3d.js.

Again, In my opinion: for r89 we should keep them how they are now.
We can reopen this subject after r90 release.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

In my opinion: for r89 we should keep them how they are now.
They identified stack overflows for large strings in old browsers, they will come with more fixes,
so It's better to rely on the THREE functions for now.

https://github.com/mrdoob/three.js/blob/8701e404874bc603b814b7ea50aaf5216cd2c9ef/src/loaders/LoaderUtils.js#L23

You're sure this work for oriental characters?

@RemusMar
Copy link
Contributor Author

You're sure this work for oriental characters?

No, I'm not.
But let's give them some time to fix all the stack overflows.
We will create

SEA3D.LoaderUtils.prototype = {

starting with r90.
Don't you agree?

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

No, I'm not.
But let's give them some time to fix all the stack overflows.

Try naming a 3d object in sea3d to 日本国 and test with the current stack overflows approach. It is not work.

@RemusMar
Copy link
Contributor Author

Try naming a 3d object in sea3d to 日本国

Can they do that in THREE ?

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

Can they do that in THREE ?

Not using this approach. Its work done to ASCII but not UTF8 (pure).

@RemusMar
Copy link
Contributor Author

We updated SEA3D for THREE.
If they cannot do it in THREE r89, is not our problem.

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

We updated SEA3D for THREE.
If they cannot do it in THREE r89, is not our problem.

No, no! SEA3D must have support to oriental characters. Three.js can use oriental characters with another approach if wish. It is important that the sea3d.js approach work done.

@RemusMar
Copy link
Contributor Author

RemusMar commented Dec 20, 2017

I tried to keep.

return decodeURIComponent( escape( String.fromCharCode.apply( null, new Uint8Array( buffer ) ) ) ); 

But they didn't like the idea.
Read again these messages:
mrdoob/three.js#12911

@sunag
Copy link
Owner

sunag commented Dec 20, 2017

Today I can not it because of lack of time, but tomorrow I am going to do a PR in threejs about it.

@RemusMar
Copy link
Contributor Author

It's better to make a PR on their LoaderUtils.
Otherwise we'll have the same problem again and again in the THREE future updates.

@RemusMar
Copy link
Contributor Author

See the latest SEA3D update.
Now it's Independent of THREE and includes your fix for multibyte characters.
cheers

@sunag
Copy link
Owner

sunag commented Dec 21, 2017

I see, I see. That is it, I will merger it soon.
Thanks!

@sunag sunag closed this Dec 21, 2017
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.

2 participants