cesium-native icon indicating copy to clipboard operation
cesium-native copied to clipboard

Consider to not throw invalid_argument exceptions

Open javagl opened this issue 3 years ago • 3 comments

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 an std::optional<Plane>. We could also consider separate createThrowing/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?".

javagl avatar Apr 13 '21 15:04 javagl

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);

kring avatar Apr 27 '21 12:04 kring

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.

javagl avatar Apr 27 '21 22:04 javagl