RobLoach / raylib-cpp

C++ Object Oriented Wrapper for raylib
https://robloach.github.io/raylib-cpp/
zlib License
584 stars 77 forks source link

Add Model(raylib::Mesh) constructor to throw an exception #305

Open RobLoach opened 3 months ago

RobLoach commented 3 months ago

@furudbat What about just throwing an exception with a hint about how to avoid the error?

furudbat commented 3 months ago

I'm not the biggest fan of Exceptions (for gamedev or resource-loading, I'm using std::expected-alikes, 'placeholder-error' textures, etc.), but if I can do something at compile-time then I prefer to catch the error at compile time. We can easily delete the ctor and those (Load) functions, if we CAN'T copied them.

I didn't find anything in raylib cheatsheet for coping the Meshes (like for Image), then we could hold the copied meshes (via MeshUnmanaged) in the Model class and own both, the model and, the model owns the meshes.

The problem I see with const MeshUnmanaged&, is still the question of the ownership, we don't know if there is an raylib:Mesh above all (who is already managing the state) or maybe other MeshUnmanaged (copies), that are basically dangling references to the Mesh in the Model (if the Model gets destroyed).

In my own solution I deleted the ctors: Model.hpp

    constexpr Model(const Model&) = delete;
    Model(Model&& other) noexcept { ...}

    constexpr Model& operator=(owner<const ::Model&> model) = delete;
    constexpr Model& operator=(owner<::Model&&> model) noexcept { ... }

I'm not sure if I should make an move-ctor for ::UnmanagedModel... I personally don't use Model, but it's pretty much the same spiel as with RenderTexture

    TextureUnmanaged GetTexture() {
        return TextureUnmanaged{m_data.texture};
    }

    void SetTexture(owner<const ::Texture&> newTexture) = delete;
    void SetTexture(owner<::Texture&&> newTexture) noexcept {
        m_data.texture = newTexture;
        newTexture = NullTexture;
    }

You can still get an TextureUnmanaged but you are not the owner, you can still do things with the texture, but it's pretty much an "read-only texture" (drawing, etc.). You can't convert them between Texture <-> TextureUnamanged, IMO either you know what you are doing and load an Texture or you load an TextureUnmanaged. I try to avoid using C raylib DataTypes (directly), either you are using the build-in Load-Functions and get an raylib::Texture or you know what you are doing and construct and raylib::Texture by moving the ownership (of ::Texture ). Or the ownership had someone else and you use the nice TextureUnmanaged-Wrapper to do things with the texture.

My approach would be, load the Model and Mesh as correct as possible, all at once:

    raylib::Model model(raylib::Mesh::Cubicmap(imMap, Vector3{ 1.0f, 1.0f, 1.0f }));
raylib::Texture texture;
// load texture
raylib::Model model = [&](){
   raylib::Mesh mesh;
   raylib::Model model;
   // load things and do stuff with texture 
   // move all possible ownership into model
   return model;
}();

create/load texture(s) and model(s) via lamdas and move them into an (managed) assetsMap (or something).

furudbat commented 3 months ago

I'm still try to figure out some things, but by deleting and forbid some (implicit) casting, all my "owning" classes, uses Composition over inheritance, I catch errors at compile time. And if you REALLY know what you are doing, you can still (explicit) convert to C raylib DataTypes [[nodiscard]] ::Texture c_raylib() const & noexcept { return m_texture; }.

And all the copy-able basic DataTypes, like ::Color, ::Rectangle, are still using inheritance, for easily passing parameters and implicit casting between C raylibs DataTypes. (Maybe I will try to avoid implicit copies for performance reasons)

But that's just my personal approach, I'm still testing things out and been open for feedback. Catch bugs and errors at compile-time.

RobLoach commented 3 months ago

Deleting it all together seems like the best option. Good call.

Regarding composition instead of inheritance, I was considering something similar in the beginning of the project as well. But in the end, thought that having the raylib-cpp objects inherit from the structs would keep the underlaying structure more similar to raylib, and still be able to interopt with it similarly.

In the end it does get tricky when a raylib object wants to take control of the memory management itself, like this Model/Mesh example. I'd love to keep the simplest solution that remains close to raylib, but provides subtle niceties on top.

RobLoach commented 3 months ago

https://github.com/RobLoach/raylib-cpp/pull/306 could be an ez first step