-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
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
isVector3 is present when serializing Vector3 #24167
Comments
This is duplicate of #24093. Serializing math objects should not be done via |
Well, that will require a LOT of change in my code. a LOT. |
I think we can solve this by adding a toJSON() {
return { x: this.x, y: this.y, z: this.z };
} |
That is one way of solving it for sure ( will probably slow down the process of serialization, which is a different issue ). But I'm really curious about the need for a |
Because every "object" class in three.js has such boolean. |
Read #24047 for more details. |
Could you do performance test? |
@mrdoob will do asap ( will probably do that tomorrow, have to work on something today ). The real question is why you need as |
@LeviPesin ok but how is it use ? |
There are occasions where we have to test for the object's type. We could do this with the |
I honestly have a really really hard time getting my head around that. For me, the less attribute you have in an heavily use class the better. I can understand for "hi level" object, but for math object, I think it should be kept to the bare minimum. But then again, it is a reflexes I had when I was using C++, so it might really not being relevant here. It just feels really wrong. Also, "there are occasion" is the root of all evil :). ( I'm benchmarking the |
Yup HEAVY hit : import { Vector3 } from "./Vector3.js"
const array = []
let i
for (i = 0; i < 100000; ++i) {
array.push(new Vector3(Math.random(), Math.random(), Math.random()))
}
const start = performance.now()
for (i = 0; i < 100; ++i) {
JSON.stringify(array)
}
const delta = performance.now() - start
console.log(`took ${delta} ms`) Without toJSON:
With toJSON:
|
@Mugen87 there is no performance impact adding import { Vector3 } from "./Vector3.js"
const array = []
let i
for (i = 0; i < 100000; ++i) {
array.push(new Vector3(Math.random(), Math.random(), Math.random()))
}
const len = array.length
const start = performance.now()
for (let j = 0; j < 10000; ++j) {
for (i = 1; i < len; ++i) {
array[i - 1].add(array[i])
}
}
const delta = performance.now() - start
console.log(`took ${delta} ms`) |
Even if a custom |
Performance wise, it is not super good, at least for my personal use case ( I'm using serializiation on backends, generating a good amount of points ). It is definitely better than nothing though, even if I really don't get the need for a boolean inside a vector class. Thanks a lot for the help ! |
@mrdoob the good old grep ! could have use that. Why not use static member ? class Vector3 {
static isVector3 = true;
...
}
const a = new Vector3()
console.log(a.constructor.isVector3)
> true
console.log(a)
> Vector3 { x: 0, y: 0, z: 0 } |
This has been discussed before, #21285 (comment) Static properties do now allow |
I agree it is not a substitute, but it seems like way better to define a static, and access it through the constructor then adding a member ? I do understand that it would require a lot of change ( since it probably mean that it needs to be implemented everywhere ), but at the same time, this seems way more clean than add a bit of memory on each vector3 ? ( I know, I'm on and on about that, but this feels really REALLY dirty to add a boolean to a vector class that will be created on each instance. Also if I look at the grep, from @mrdoob this can probably be done another way ? ) ( I'm invoking my inner Mike Acton ! ) |
Hm. Can a So it would be like the JS's own instanceof (i.e. it would not require .is booleans) and it would work with tree-shaking. |
Well ... class Vector3 {
get isVector3() {
return true
}
...
}
const a = new Vector3()
console.log(a.isVector3)
> true
console.log(a)
> Vector3 { x: 0, y: 0, z: 0 }
|
The migration effort that results from such a change is inacceptable. Besides, the additional memory allocation of a single boolean is so minor that the resulting overhead is negligible, see #24047 (comment). Turning the |
I'm not to sure about that. at the bare minimum it's one byte per item, but given the proportion of javascript to suck up memory : import { Vector3 } from "./Vector3.js"
const array = []
for (let i = 0; i < 1000000; ++i) {
array.push(new Vector3())
}
const used = process.memoryUsage().heapUsed / 1024 / 1024;
console.log(`The script uses approximately ${Math.round(used * 100) / 100} MB`); With isVector3 as an attribute : Without: This adds about 10 mega bytes of memory for a million vector3. I would not call that negligible ! Side note, using accessor ( get version ) is way better : The script uses approximately 65.65 MB |
But slow (also as my version). I think that the public fields (proposed by @marcofugaro) is actually a good idea? It will create only one instance per class. |
It's a shame there isn't a way with class definitions to declare fields on the prototype... I haven't seen this approach addressed for the class Vector3() {
constructor() {
Object.defineProperty( this, 'isVector3', { value: true } );
this.x = 0;
this.y = 0;
this.z = 0;
}
} Not sure if there's an performance implications to the above approach but it does prevent |
It's also the case that if @olivierchatry's use case is considered too unique that they can override the toJSON function themselves for their own app: Vector3.prototype.toJSON = function() {
return { ... };
}; |
|
@Mugen87 okay thanks for the reference. Last crazy idea, though - what if we set the prototype in the class constructor? I'm not sure how this would affect tree shaking and I don't love the idea but it would address both of the concerns raised in this thread (new class Vector3() {
constructor() {
Vector3.prototype.isVector3 = true;
// or
Object.getPrototypeOf( this ).isVector3 = true;
this.x = 0;
this.y = 0;
this.z = 0;
}
} My hope would be that since the prototype access is within its own constructor that it won't affect tree shaking but I suppose that might be fairly bundler-implementation specific so maybe it's a risky assumption. |
I think we can actually try using polymorphism and "does the object have the method we need" (duck typing) instead of using |
I'm worried that this thread seems pretty far down a very niche path... THREE.Vector3 does not need to be optimized around storing millions of Vector3 instances in memory — that's a terrible choice for performance regardless of this particular boolean property. This is the reason why three.js (and GPU APIs) have moved toward typed-array-based APIs like BufferAttribute instead. If you need to iterate over many vertices, a small number of vectors should be pooled and initialized, when needed, from compact binary views. Let's keep |
I can of disagree with that. ArrayBuffer are actually slower access wise if you do work on the CPU ( don't ask me, it's horrible, we tested changing some of our generator to use arraybuffer, and it ended up a good chunck slower than using Vector3 ). I'm sure when WebGPU will be there we can work something out, but at least in our case Vector3 is actually faster than working against arraybuffer. |
@donmccurdy meant using one TypedArray for all Vectors3, not a TypedArray for each Vector. It would be much, much faster. |
That is what I was doing. If you want I can certainly redo the benchmark and so you can see. We event tried using indexed array instead of floating point to limit writes size. It was slower. |
( did you actually do any benchmark your self ? because I was blown away myself by the result. I asked our render coder to modify our code to use typedarray so that we could leverage some webassembly, and the result were shocking to the point that I reimplemented it myself to be sure. So yeah, typedarray access on pure JS code is slower, at least in my test. ) |
Here it is: |
This is really not a good bench. You are benchmarking the call to a vector3 constructor. Also, linear access. I'm redoing the bench I have made using simplify from there : https://mourner.github.io/simplify-js/ |
Well, never mind, we did something really stupid ( I removed the For science: With
Without
We were comparing vec3 with |
This is a "best case scenario" access. On complex algo, with random access,
it is different. Let me redo my bench.
…On Thu, Jun 9, 2022 at 6:13 PM Levi Pesin ***@***.***> wrote:
Here it is:
https://jsbench.me/52l477mmlz/2
TypedArray is more than 10x faster.
—
Reply to this email directly, view it on GitHub
<#24167 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAJVOGZWGGO3YSXXENYAAH3VOIJZFANCNFSM5XQFYMYA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Describe the bug
A clear and concise description of what the bug is. Before submitting, please remove unnecessary sections.
since r141, when serializing Vector3 class using JSON.stringify,
isVector3
is presentTo Reproduce
Steps to reproduce the behavior:
Code
Live example
*141
*140
Expected behavior
Should not contains
isVector3
( also, I don't think it is a good idea to have a boolean inside an heavily use class memory wise )Screenshots
Platform:
The text was updated successfully, but these errors were encountered: