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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RobLoach
Copy link
Owner

@RobLoach RobLoach commented Mar 9, 2024

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

include/Model.hpp Outdated Show resolved Hide resolved
@furudbat
Copy link
Contributor

furudbat commented Mar 9, 2024

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
Copy link
Contributor

furudbat commented Mar 9, 2024

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
Copy link
Owner Author

RobLoach commented Mar 9, 2024

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
Copy link
Owner Author

#306 could be an ez first step

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

Successfully merging this pull request may close these issues.

2 participants