-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Do cross, normalize, etc. need to mutate the original vectors? #169
Comments
This bit me too; I prefer non-mutating defaults with explicit mutating variants! I like the ruby convention of having “!” variants be mutating. For compatibility... Maybe a separate package, or some module level flag to opt into new behavior? |
Yeah, I think this is a tricky area. The two different systems (mutable and immutable) each have tradeoffs, and I don't know if one is "better". I think at this point I'm used to the mutations but have probably just internalized the paranoia you describe and developed a bit of stockholm syndrome. If I take a step back, I can see how avoiding mutations altogether would make the API easier to use in some ways. On the other hand, it's a bit nice knowing that vectors are only created when There's also a limited number of temporary vectors (16k at the moment). It's possible that encouraging more vector allocations would cause projects to bump up against this limit more often, and I'd want to make sure that the workaround for this isn't more annoying than just having mutable vectors be the default. Mutable variants of functions is one possibility, another one might be optional out-args? (It's worth saying that the mutable vectors can run out of temp space too). I'd also like to do some simple profiling to see what the performance characteristics between mutable and immutable are. As you said, the temp vector system should mean that they're pretty much the same, but sometimes there can be surprises in real projects. So...I'm not sure. Not strongly opposed to switching to a more immutable model. The breaking change aspect of it is a little unfortunate, especially since vectors were reworked this last version, but it can still be done. But I want to be cautious and make sure that people won't open another issue in a few months saying how the vector API encourages too many allocations and we should switch to mutable-by-default :) The "separate package" idea is interesting -- if there are several different equivalent approaches for doing vectors (FFI vs. lightuserdata, immutable vs. mutable) then maybe LÖVR could just define conventions for how vectors can get passed into the API and then people could bring their own favorite conforming math library. Thank you for starting this discussion! |
Would it be possible to allow people to control the size of the temporary vector pool? If you have that, and the ability to drain the pool, hopefully it might not be too much of an issue. From a philosophical standpoint, I don't think mutations are consistent with math operations in general: x = 3
y = x + 5
-- we don't expect x to be set to 8!
-- furthermore, if + mutated its first value, this would no longer make sense:
z = 3 + 5
-- because how could 3 be mutated? Operations like That said, I know efficiency is an important concern, which is why I think it would be good to preserve the mutation API in some way. But I just don't think it makes sense for mutation to be the default. Long-term, I like the idea of defining vector and quat "interfaces" that allow people to use their own favorite math library. But I think that's mostly a separate concern from the behavior of the default API, since the default API is what most people will start with, and probably use long-term. |
I've adapted this style of working with math heavy graphics: local tvec1, tvec2 = vec3(), vec3()
local tmat = mat4()
for i, mesh in ipairs(geometry) do
tmat:target(tvec1:set(...), tvec2:set(...))
mesh:draw(tmat)
end Temporary vectors are reused in iterations and I only need a few of them so I never run out. The hot code inside of loop doesn't allocate. It relies heavily on mutation though. If vector operations would return temporary vectors, I could no longer use this style. With out-args the code would look terrible and I would switch away to something more sane. Having non-mutating variants for each function sounds good, but most of operations are already covered with metamethods. Maybe we should focus on those not covered (normalize, cross, lerp, distance) and make non-mutating variants for them. Lastly, I think math library should remain included in the framework. It is valuable to have a good default library for fundamental operations in computer graphics. Makes it possible to write examples for docs and to support the community questions, as quite a few are about math. |
I'm new to 3D math, wrapping my head around the math functions, and I'm constantly getting bitten by my assumption that they don't mutate. I wish I could do Though it would be wordier, I'm also surprised that there's no Edit, a week later: I think the mutating vectors somewhat fit into my intuition for commands like If the mutating vectors do remain the default, then |
PR #731 is a code change under consideration that resolves this issue. |
I've been bitten several times over the past few days by vector mutations leaking into places I didn't expect. It seems that some operations yield new temporary vectors, while others just mutate the first argument and return it for chaining. It is not clear which operations do this, so I've found myself wrapping lots of things in
vec3
out of paranoia.While I understand that a mutation-based API might be good for performance reasons, I don't think it's a great default behavior, since it leads to sneaky issues that are hard to debug. I also don't think it's really necessary for most things, since you have this temporary vector system that reduces allocation and GC load.
At the moment, my preferred API design would be:
:mutCross
,:mutNormalize
, etc. Or something along those lines. This makes it easy to opt into mutation for cases where it matters, like with permanent vectors that you are frequently changing.However, I understand that this would be a big change that is liable to break, well everything. I just want to bring it up for discussion and get your thoughts on it.
The text was updated successfully, but these errors were encountered: