jeremyletang / rust-sfml

SFML bindings for Rust
Other
638 stars 88 forks source link

Implementing RcTexture and RcSprite #298

Closed dogunbound closed 1 year ago

dogunbound commented 1 year ago

Inspiration for this was to allow sprites and texture lifetimes to separate from each other. Currently, the lifetimes do not qualify for texture and sprites to be discontinuous, and it is challenging to program complicated resource management systems around this contingency.

This PR will allow users to safely separate textures and sprites without too much worry about UB. This setup is more synonymous with the main SFML setup.

NOTE: Main SFML considers using sprites while the texture object is NULL as UB.

The only real issue is the transform functions:

If I do not prevent users from transforming a sprite while the texture is not alive, then according to eXplOit3r, that is UB. If I do prevent users from transforming a sprite while the texture is not alive, then transformations will seemingly disappear. Right now (with the current PR), RcSprite will do UB if the texture is not alive. This doesn't mean it will crash, but I simply do not know what the sprite will do if a transformation is done while the texture is not alive. I do know that setPosition won't crash, but I have not tested all the other transformations nor do I think we should.

There are many solutions to this problem:

  1. Prevent transformations if the texture is no longer alive
  2. Warn users in the documentation that modifying a RcSprite is UB if the RcTexture is no longer alive, and simply blame the user for not reading the documentation.
  3. Setup tests to see which RcSprite transformation functions display malicious UB while the RcTexture is no longer alive.
  4. Use strong pointers (Rc) to the texture pointer inside RcSprite. This will allow the deletion of RcTexture because the underlying texture will still be alive, and will only die once all RcSprite objects that point to that texture have also died. This would allow RcSprite to continue to safely draw, transform, etc., but this is very much against the grain of SFML. Doing this would essentially change the behavior sprites exhibit from the main SFML. NOTE: The disable_texture function would have to go if we want to do this as this would allow users to perform UB, or we could just warn and blame the users for incorrectly using this function.
  5. Return a result on transformations as to whether they were successful. This would require adding some error handling to the transform functions and returning a result for EVERY transform function. This sounds like a pain and will make the interface ugly to work with.

My suggested option is 2

dogunbound commented 1 year ago

I want you to do a code review and give a selection as to what option (or maybe even your own idea) for what to do about transformations and UB @crumblingstatue

dogunbound commented 1 year ago

After discussion, we decided to simply log, or panic if functions are called when the texture is no longer alive.

crumblingstatue commented 1 year ago

Thank you for your contribution!