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

SEA3DLoader: Morph Normals fix and extractUrlBase update #12911

Merged
merged 4 commits into from Dec 19, 2017

Conversation

Projects
None yet
4 participants
@RemusMar
Contributor

RemusMar commented Dec 19, 2017

Morph Normals fix and r89 extractUrlBase update

RemusMar added some commits Dec 19, 2017

Morph Normals fix and extractUrlBase update
Morph Normals fix and r89 extractUrlBase update
@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

Here is the new minified version in action:
http://necromanthus.com/Test/html5/SMC.html

Contributor

RemusMar commented Dec 19, 2017

Here is the new minified version in action:
http://necromanthus.com/Test/html5/SMC.html

@Mugen87

This comment has been minimized.

Show comment
Hide comment
@Mugen87

Mugen87 Dec 19, 2017

Collaborator

Any ideas why you have such a big diff? Have you used a different formatting (hard vs. soft tabs)?

Collaborator

Mugen87 commented Dec 19, 2017

Any ideas why you have such a big diff? Have you used a different formatting (hard vs. soft tabs)?

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

Any idea why you have such a big diff? Have you used a different formatting

There were some major changes (fixes) in the past 10 months.
But yes, I think Github shifted the context somehow (I've uploaded the new files).

Contributor

RemusMar commented Dec 19, 2017

Any idea why you have such a big diff? Have you used a different formatting

There were some major changes (fixes) in the past 10 months.
But yes, I think Github shifted the context somehow (I've uploaded the new files).

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

If you "Load diff" you will see there is no "diff" in most of the cases.
LOL

Contributor

RemusMar commented Dec 19, 2017

If you "Load diff" you will see there is no "diff" in most of the cases.
LOL

SEA3D update
SEA3D update
@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

I've uploaded the files again and now there are no differences (as expected).
First time, they posted wrong differences because they added a new line to every file:

--- @@ -1,3158 +1,3317 @@

So for the SEA3D.js there were only 159 differences.

Contributor

RemusMar commented Dec 19, 2017

I've uploaded the files again and now there are no differences (as expected).
First time, they posted wrong differences because they added a new line to every file:

--- @@ -1,3158 +1,3317 @@

So for the SEA3D.js there were only 159 differences.

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Dec 19, 2017

Collaborator

Apparently I missed SEA3D loader in #12654, sorry. You may want to replace these lines:

if ( window.TextDecoder ) {

	return new TextDecoder().decode( buffer );

} else {

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

}

with:

return THREE.LoaderUtils.decodeText( new Uint8Array( buffer ) );

This should also avoid stack overflows for large strings in old browsers. Although decodeText doesn't have the decodeURIComponent( escape( ... ) ) wrapper... is that indented to strip characters that weren't decoded correctly?

Collaborator

donmccurdy commented Dec 19, 2017

Apparently I missed SEA3D loader in #12654, sorry. You may want to replace these lines:

if ( window.TextDecoder ) {

	return new TextDecoder().decode( buffer );

} else {

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

}

with:

return THREE.LoaderUtils.decodeText( new Uint8Array( buffer ) );

This should also avoid stack overflows for large strings in old browsers. Although decodeText doesn't have the decodeURIComponent( escape( ... ) ) wrapper... is that indented to strip characters that weren't decoded correctly?

@donmccurdy donmccurdy changed the title from Morph Normals fix and extractUrlBase update to SEA3DLoader: Morph Normals fix and extractUrlBase update Dec 19, 2017

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

Maybe this approach is better:

if ( window.TextDecoder ) {
	return THREE.LoaderUtils.decodeText( new Uint8Array( buffer ) );
} else {
	return decodeURIComponent( escape( String.fromCharCode.apply( null, new Uint8Array( buffer ) ) ) );
}

What do you think?

Contributor

RemusMar commented Dec 19, 2017

Maybe this approach is better:

if ( window.TextDecoder ) {
	return THREE.LoaderUtils.decodeText( new Uint8Array( buffer ) );
} else {
	return decodeURIComponent( escape( String.fromCharCode.apply( null, new Uint8Array( buffer ) ) ) );
}

What do you think?

@donmccurdy

This comment has been minimized.

Show comment
Hide comment
@donmccurdy

donmccurdy Dec 19, 2017

Collaborator

decodeText already contains a fallback for browsers without TextDecoder, and in GLTFLoader we've found String.fromCharCode.apply to throw RangeErrors for large buffers... I am hoping to understand what the non-URI-compatible-character-stripping is for, and then to add that (or something equivalent) in decodeText so that other loaders can use it. See: https://stackoverflow.com/questions/8936984/uint8array-to-string-in-javascript ... I'm guessing that if the fallback decoding could handle utf-8, that wouldn't be necessary.

Collaborator

donmccurdy commented Dec 19, 2017

decodeText already contains a fallback for browsers without TextDecoder, and in GLTFLoader we've found String.fromCharCode.apply to throw RangeErrors for large buffers... I am hoping to understand what the non-URI-compatible-character-stripping is for, and then to add that (or something equivalent) in decodeText so that other loaders can use it. See: https://stackoverflow.com/questions/8936984/uint8array-to-string-in-javascript ... I'm guessing that if the fallback decoding could handle utf-8, that wouldn't be necessary.

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 19, 2017

Contributor

OK

Contributor

RemusMar commented Dec 19, 2017

OK

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Dec 19, 2017

Owner

@Mugen87

Any ideas why you have such a big diff? Have you used a different formatting (hard vs. soft tabs)?

Sometimes saving the file in a different OS messes things.

Appending ?w=1 to the url helps to see what actually changed:
https://github.com/mrdoob/three.js/pull/12911/files?w=1

Owner

mrdoob commented Dec 19, 2017

@Mugen87

Any ideas why you have such a big diff? Have you used a different formatting (hard vs. soft tabs)?

Sometimes saving the file in a different OS messes things.

Appending ?w=1 to the url helps to see what actually changed:
https://github.com/mrdoob/three.js/pull/12911/files?w=1

@mrdoob mrdoob merged commit d81cb1e into mrdoob:dev Dec 19, 2017

2 checks passed

lgtm analysis: JavaScript No alert changes
Details
lgtm analysis: Python No alert changes
Details
@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Dec 19, 2017

Owner

Thanks!

Owner

mrdoob commented Dec 19, 2017

Thanks!

@mrdoob

This comment has been minimized.

Show comment
Hide comment
@mrdoob

mrdoob Dec 19, 2017

Owner

Fixes #12909 I assume?

Owner

mrdoob commented Dec 19, 2017

Fixes #12909 I assume?

@RemusMar

This comment has been minimized.

Show comment
Hide comment
@RemusMar

RemusMar Dec 20, 2017

Contributor

Fixes #12909 I assume?

Already included.

cheers

Contributor

RemusMar commented Dec 20, 2017

Fixes #12909 I assume?

Already included.

cheers

@RemusMar RemusMar referenced this pull request Dec 20, 2017

Closed

Updated to THREE r89 #11

@sunag sunag referenced this pull request Dec 20, 2017

Closed

LoaderUtils.decodeText non UTF8 #12933

2 of 2 tasks complete

@mrdoob mrdoob added this to the r90 milestone Jan 17, 2018

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