search

zesterer / euc

A software rendering crate that lets you write shaders with Rust
Apache License 2.0
294 stars 15 forks source link

Always cloning in get() to allow for any underlying representation #3

Closed sunjay closed 5 years ago

sunjay commented 5 years ago

I ran into a problem today and this PR aims to help address that.

I actually noticed that the documentation for get() in the Target trait in some ways reflects the change I'm currently proposing: (emphasis mine)

Get a copy of the item at the specified location in the target. The validity of the location is not checked, and as such this method is marked unsafe.

It says "copy", but the get() method currently returns a reference. Furthermore, if you search for get() in the repo, all its usages are immediately dereferenced. That means that we could potentially just return a clone and remove those dereferences.

This PR changes the get() method in the Target trait to return Self::Item instead of &Self::Item. It then makes any related changes to other parts of the code.

Use Case

The reason I'm suggesting this is because I actually ran into a situation today where I really need this trait to return Self::Item instead of a reference.

The problem I have is in some FFI code. I have some shared memory represented as Vec<u8> and I use a euc::Pipeline implementation that returns vek::Rgba<f32>. The Vec<u8> is laid out in quadruples of (r, g, b, a).

Rather than maintaining a separate Buffer2d<Rgba<f32>>, I want to avoid copying and just use the Vec<u8> directly. I can implement set() very easily, but implementing get() is impossible because I can't return a &Rgba<f32> from &[u8]. I can however return Rgba<f32>.

The changes in this PR would make euc usable for me in this case.

Impact of this change

I think the clone has minimal cost in almost all circumstances and will probably be optimized away. The only time get() is used in the various rasterizers is for the depth buffer anyway. That almost always has type Buffer2d<f32> and cloning an f32 should be very cheap.

This would be a breaking change, so 0.3 would need to be released, but hopefully that is no big deal.

If you decide to merge this PR and my other one (#2) with no changes required, could you please release a new 0.3 of euc? Thank you :)

zesterer commented 5 years ago

This is a good change and an issue with the original implementation that I was uneasy about at the time. Thanks for fixing it. I'll release 0.3 to coincide with these changes.

All that needs to happen is for the conflicts to be resolved.

sunjay commented 5 years ago

@zesterer Done! Thank you for reviewing my changes and publishing that release. :smile:

zesterer commented 5 years ago

@sunjay Feel free to add your name to the author list!

sunjay commented 5 years ago

@zesterer I'll do that in the next PR (someday) :smile: Thanks for merging my PRs! Looking forward to 0.3. :tada:

zesterer commented 5 years ago

@sunjay 0.3 was just published!

sunjay commented 5 years ago

Awesome! Thank you! :tada: :tada: :tada: