novelrt / NovelRT

A cross-platform 2D game engine accompanied by a strong toolset for visual novels.
MIT License
183 stars 43 forks source link

Consider migrating all `Try` methods to return `std::optional<T>` instead of `bool` with some kind of out `T&`. #563

Open RubyNova opened 1 year ago

RubyNova commented 1 year ago

Note: for support questions, please use the #engine-user-help channel in our Discord or create a discussion. This repository's issues are reserved for feature requests and bug reports.

What is the current behaviour? Try methods are currently formatted as bool TryThing(T& outResult). This means users can invoke them and check the bool to see if it succeeded or not, and access the T& in the case of success.

What is the expected behaviour/change? The change would mean the methods would now be formatted as std::optional<T> TryThing(), which would allow users to still perform the same checks as before.

What is the motivation / use case for changing the behavior? The API will be clearer on intent as there is no lingering T& which has to be explained via documentation for those who have not seen that style before. This is a carry-over from C# in NovelRT's original desgin when many of us were more familiar with the dotnet ecosystem, and how they handle this particular case in older APIs. However we are under no such restriction here, and so I believe relying on the standard library's optional type will provide more clarity to end-users.

Describe alternatives you've considered: I have considered simply not doing this and leaving the API as-is, however since functionally it would be the same behaviour I believe it should at least be considered.

I have also considered simply wrapping what we have now in helper methods that return std::optional<T> but I think that would be pointless as all we'd effectively be doing is adding another method in the call stack every time its invoked.

Are there any potential roadblocks or challenges facing this change? The C API mirror for all affected methods would need revisiting - either in implementation, or the implementation + the signature structure. It will also break any internal usages of these methods, meaning all affected code would need to be adjusted so that things compile again.

Are there any downsides to implementing this change? Mostly the boilerplate work outlined in the roadblocks section.

Additional context I've shown a working example in #564 on what the APIs would look like in a real world use case.

Pheubel commented 1 year ago

In general i think it would allow for a more straight forward usage of the API, as newcomers to C++ might not be familiar with where the value suddenly comes from (it's like black magic!).

it would change

ResultType result;
if (TrySomething(result))
    UseResult(result);

to something more expressive in intend.

std::optional<ResultType> result = TrySomething();
if (result.has_value())    // same as "if (result)"
    UseResult(result.value());

The moment that multiple things are returned as a result of a try method, then i think it might be a bit more tricky, as most likely will have to use a wrapper object for that.

one consideration might be how easy it is to copy around the std::optional<T>, i'd imagine that for NovelRT i shouldn't be that big of a deal, but i don't know the answer for that.

RyadaProductions commented 1 year ago

I will pick this one up.