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

Do cross, normalize, etc. need to mutate the original vectors? #169

Open
bvisness opened this issue Nov 21, 2019 · 6 comments
Open

Do cross, normalize, etc. need to mutate the original vectors? #169

bvisness opened this issue Nov 21, 2019 · 6 comments
Labels

Comments

@bvisness
Copy link

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:

  • All vector operations that return a vector yield a new temporary vector, both for permanent and temporary vectors. (This includes operator overload stuff.)
  • The existing set of mutate-y behaviors is renamed to :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.

@nevyn
Copy link
Contributor

nevyn commented Nov 22, 2019

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?

@bjornbytes
Copy link
Owner

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 vec3 is called (metamethods are an exception to this though...).

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!

@bvisness
Copy link
Author

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 |x| or x cross y are just like + and don't cause mutations, so it's pretty confusing when the :whatever() equivalent doesn't have the same behavior.

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.

@jmiskovic
Copy link
Contributor

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.

@KelseyHigham
Copy link
Contributor

KelseyHigham commented Apr 6, 2022

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 :setLerp to mutate, :lerp to return a temporary vector.

Though it would be wordier, I'm also surprised that there's no lovr.math.lerp(Vec3, Vec3, t) function to supplement the metamethods.


Edit, a week later: I think the mutating vectors somewhat fit into my intuition for commands like *= and += in other languages, which Lua notably doesn't have. I continue to think that :mul = *, :setMul = *= is an intuitive syntax.

If the mutating vectors do remain the default, then :lerp to mutate, :getLerp to return a temporary vector would be more helpful than not having :getLerp.

@bjornbytes
Copy link
Owner

PR #731 is a code change under consideration that resolves this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants