cesium-native
cesium-native copied to clipboard
Consider to not throw invalid_argument exceptions
Right now, there are few places where an invalid_argument
exception can be thrown:
- In the
Plane
constructor, when the given normal is not normalized ( https://github.com/CesiumGS/cesium-native/blob/8a2957d32f433438b6b91ad37059c1ca0c8665c5/CesiumGeometry/src/Plane.cpp#L13 ) - In the
Ray
constructor, when the ray direction is not normalized ( https://github.com/CesiumGS/cesium-native/blob/8a2957d32f433438b6b91ad37059c1ca0c8665c5/CesiumGeometry/src/Ray.cpp#L13 ) - In the
EllipsoidTangentPlane
class, when the origin is near the center ( https://github.com/CesiumGS/cesium-native/blob/b824699d3dd9503a5943007247f683b6adc132d4/CesiumGeospatial/src/EllipsoidTangentPlane.cpp#L51 )
As much as I'm a fan of early, clear error checks and responses, we should consider handling this differently: Recent crashes in Unreal ( https://github.com/CesiumGS/cesium-unreal/issues/290 and https://github.com/CesiumGS/cesium-unreal/issues/336 ) have been caused by this, and these crashes are hard, painful, and non-recoverable.
I'm not sure about the best solution for this.
- Simple solutions (like filling stuff with
NaN
or so) are certainly not appropriate, becauses these will have the effect of "Nothing is rendered", without any clue of what's happening at all. - Enforcing to wrap the creation of these objects into
try-catch
will be hard (and inconvenient) - More strict, invasive solutions could be (brainstorming) to make the constructors of these classes
private
, and only allow them to be created with factory methods that return anstd::optional<Plane>
. We could also consider separatecreateThrowing
/createUncheckedWithNaN
/createOptional
factory methods.
I just did a websearch about what e.g. GLM does for a normalize
call on zero-length vectors, but... surprisingly there is no obvious information for that (have to try it out, I guess). The point is that we should consider "following conventions" for such cases ... if there are conventions. Otherwise, it largely depends on the desired and expected behavior, and the crucial question of "What should happen when the precondition does not hold?".
There's no great solution, but really the only kinda-ok one is to have alternate ways to indicating failure. That might mean logging or returning a struct with errors and warnings, and we do both of those things in different situations in cesium-native already. But for low-level types like the examples in this issue, the best approach I know of is what Microsoft in the C# world calls the "TryParse" pattern: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/exceptions-and-performance?redirectedfrom=MSDN#try-parse-pattern
In short, provide a method prefixed with try
that indicates success or failure with its return value. In C# they use a bool return and and out
parameter, but in C++ we can relatively efficiently accomplish the same thing by returning a std::optional
. So something like:
std::optional<Plane> plane = Plane::tryConstruct(normal, distance);
or
std::optional<glm::dmat4> enuToFixed = ellipsoidTangentPlane.tryComputeEastNorthUpToFixedFrame(origin, ellipsoid);
That tryConstruct
essentially the createOptional
that I mentioned, and I think that could be reasonable solution. For some cases, having a constructUnchecked
that does 1. assume the input to be valid or 2. does not care about NaN and such may also be OK, but can be added at any point in time.
Referring to the https://github.com/CesiumGS/cesium-native/issues/212 issue: There are a few places where an std::runtime_error
is used, but these may not always be so easy to change, and can probably be addressed separately.