-
-
Notifications
You must be signed in to change notification settings - Fork 94
feat: make Vec2 able to do math on noninstances #968
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
base: master
Are you sure you want to change the base?
feat: make Vec2 able to do math on noninstances #968
Conversation
commit: |
lajbel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
mflerackers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is there a reason to call it SerializedVec2, and not Vec2Like or something similar?
- Why is sometimes T used and sometimes SerializedVec2?
- How did chrome react? Don't these methods suddenly become de-optimized, as the argument types are no longer fixed?
No. I kind of wanted to change it to Vec2Like but didn't want to change everything since X.serialize() typically returns SerializedX and Vec2 does this too.
The type parameter is for the inout mutating methods, that mutate the object and then return the same one you passed in. That way, if you use the return value it will be the same type as the one you passed in (if it's a Vec2 instance the return value will be that type too, if it is just a Vec2Like then so will the return value).
I didn't notice any performance penalty, and it really shouldn't anyway, since all I changed were the types for the most part and by the time chrome sees the code the types are long gone. |
|
What I mean is, someone doing Vec2.add(v, w, v) with w a vertex { x, y, u, v, r, g, b, a } will cause the Vec2.add be deoptimized. |
Well, I mean yeah that would, but I doubt that will ever happen considering that that flattened format isn't even how kaplay stores verticies: /**
* @group Rendering
* @subgroup Shaders
*/
export interface Vertex {
pos: Vec2;
uv: Vec2;
color: Color;
opacity: number;
} |
|
Yes, It was just an example. I'm going to change that vertex format btw, once I rewrite the renderer. |
|
Typescript types will always let you do a lot of silly things that you really shouldn't. This was just to let you do some things that you ideally should be able to do (in my opinion at least...) but would get typescript yelling at you before. |
|
I don't have anything against the feature itself, I just want to know whether there would be any unexpected issues. |
now all Vec2 methods can be given a SerializedVec2 as arguments, so you don't have to deserialize everything
(for example:
JSON.parse(JSON.stringify(vec2(...).serialize())) instanceof Vec2 === false)Summary