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

creating Vectors with NaN values replaces most of them with 0 #19649

Closed
2 of 12 tasks
sjpt opened this issue Jun 13, 2020 · 9 comments
Closed
2 of 12 tasks

creating Vectors with NaN values replaces most of them with 0 #19649

sjpt opened this issue Jun 13, 2020 · 9 comments

Comments

@sjpt
Copy link

sjpt commented Jun 13, 2020

Setting any THREE.Vectorx with NaN values causes the NaN value to be replaced by 0,
except for the w field of a Vector4.
eg
new Vector3(NaN, 3, 4) returns Vector3(0, 3, 4)
new Vector4(NaN, NaN, NaN, NaN) returns Vector4(0, 0, 0, NaN)

I can see fixing this would add a fraction to the common code path,
but it causes some very unexpected failure to propagate NaN values.

Three.js version
  • Dev
  • [x ] r117
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@mrdoob
Copy link
Owner

mrdoob commented Jun 13, 2020

I guess we can fix this by doing this here?

this.w = ( w !== undefined ) ? w || 0 : 1;

@WestLangley
Copy link
Collaborator

three.js does not support NaNs. I do not think three.js should be responsible for converting them to zeros or ones.

NaNs should be handled at the app level, IMO -- and not passed into constructors.

@sjpt
Copy link
Author

sjpt commented Jun 13, 2020

The handling of w was (relatively) correct as it was. It was the handling of x, y, z that is at issue.

A question is if you still want to let through other values ('', null, etc) to become 0? It could break other apps if you stop allowing those. I was very surprised to find that NaN || x returns x, but I see it makes sense in some curious way and is certainly as defined.

The best I can think of that gives what I consider to be the 'appropriate' answer is
this.x = x == 0 ? 0 : x;
I don't know performance implications, probably trivial compared to the new object overhead.

Sort of related, it would be nice if renderer.setRenderTarget() set renderTarget to null.

(Reminds me why I was so anti database null values back in the 70's.)

When you say 'I do not think three.js should be responsible for converting them to zeros or ones.', I say 'I don't think three.js should irresponsibly take it upon itself to convert them to zeros or ones'.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2020

Sorry, but you make it unnecessarily complicated. App level code has to ensure to pass numbers to Vector3 or Vector4. Besides, three.js does in general not sanitize arguments.

The current code in the ctor just ensures default values. When converting to classes, the ctor can be updated too so it will look like so:

class Vector3 {

	constructor( x = 0, y = 0, z = 0 ) {

		this.x = x;
		this.y = y;
		this.z = z;

	}
}

@sjpt
Copy link
Author

sjpt commented Jun 14, 2020

I was passing a number. typeof NaN is 'number'.
Converting one number (NaN) to another (0) is not sanitizing, its polluting.

I'm pleased to say that your new code will have the correct effect and accept NaN values.

Is there any reason to wait for conversion to classes;
can't you just use Vector3( x = 0, y = 0, z = 0 ) , Vector4( x = 0, y = 0, z = 0, w = 1 ) etc now, or will that break on some old version of Javascript that you still support?
Failing that this.x = x === undefined ? 0 : x.

By the way, I'm not trying to pick holes in three.js just for the sake of it; it is an excellent system. Just with my slightly unusual usage finding and reporting bits that unexpectedly don't work as they logically might. I have a genuine need for NaN values in shaders and rendertarget/textures (to get round the lack of geometry shaders in WebGL), When I read these back for debug I can see the NaN values in the raw texture data, but putting them through a utility to make the read back textures more inspectable Vector4 values then the Nan values were polluted. When I didn't see expected NaN values (except in the .w field) I naturally assumed my shader was wrong.

It took quite a bit of debug time to track down the issue. As usual the fix was trivial; I used new Vector4().set(x,y,z,w). The reason I think it should be fixed within three.js rather than outside is that others may also waste time debugging under similar circumstances. Whereever possible a system should behave as users would expect, and converting NaN to 0 is not expected.

Sadly javascript is a pretty horrible language in many ways which makes consistency harder to achieve than it ought to be.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 14, 2020

Even if NaN is of type number, the name implies that it is not a number. Hence, it's not a valid argument.

Is there any reason to wait for conversion to classes;

Yes, see #17425 (comment).

@Mugen87 Mugen87 closed this as completed Jun 14, 2020
@sjpt
Copy link
Author

sjpt commented Jun 14, 2020

The NaN name implies it is not a number but actually means it isn't a normal number; the typing says it is. There is a well known convention for NaN handing and propagation in hardware and all programming languages that support NaN (including javascript) which three.js is breaking.

#17425 indicates a reason to defer the conversion to classes, but does not give a reason not to change the current function style code to function Vector4( x=0, y=0, z=0, w=1 ) style defaulting.

@mrdoob
Copy link
Owner

mrdoob commented Jun 16, 2020

#19678

@mrdoob
Copy link
Owner

mrdoob commented Jun 16, 2020

I was passing a number. typeof NaN is 'number'.

This made my blood boil for so many reasons...

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

No branches or pull requests

4 participants