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
base: master
from

Conversation

Projects
None yet
2 participants
@RemusMar
Contributor

RemusMar commented Dec 20, 2017

No description provided.

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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

Owner

sunag commented Dec 20, 2017

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

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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

Contributor

RemusMar commented Dec 20, 2017

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

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

In this case, we should extractUrlBase in SEA3D.js

Owner

sunag commented Dec 20, 2017

In this case, we should extractUrlBase in SEA3D.js

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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( '/' ) + '/';
}
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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 );

}
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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

"extractUrlBase" does not exist in the SEA3D files.

Contributor

RemusMar commented Dec 20, 2017

"extractUrlBase" does not exist in the SEA3D files.

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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.

Contributor

RemusMar 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.

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

"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.

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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.

Contributor

RemusMar commented Dec 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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?

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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?

Contributor

RemusMar commented Dec 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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.

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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

Can they do that in THREE ?

Contributor

RemusMar commented Dec 20, 2017

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

Can they do that in THREE ?

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

Can they do that in THREE ?

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

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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

Contributor

RemusMar commented Dec 20, 2017

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

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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.

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 20, 2017

Owner

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

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

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

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.

Contributor

RemusMar commented Dec 20, 2017

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.

@sunag sunag referenced this pull request Dec 20, 2017

Closed

LoaderUtils.decodeText non UTF8 #12933

2 of 2 tasks complete
@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 21, 2017

Contributor

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

Contributor

RemusMar commented Dec 21, 2017

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

@sunag

This comment has been minimized.

Show comment
Hide comment
@sunag

sunag Dec 21, 2017

Owner

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

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