godot-rust / gdnative

Rust bindings for Godot 3
https://godot-rust.github.io
MIT License
3.61k stars 210 forks source link

Enforce `Plane` invariants via private fields & setters #994

Open chitoyuu opened 1 year ago

chitoyuu commented 1 year ago

Created during survey of commented code (#377).

There are a number of invariants expected mathematically of the Plane type, that cannot currently be enforced due to the fields being public.

A few issues to consider:

A test should also be added for contains_point_eps.

Bromeon commented 1 year ago

There are a number of invariants expected mathematically of the Plane type, that cannot currently be enforced due to the fields being public.

This is also true to a lesser extent for Basis, Transform2D and Transform:


Should validation be performed when getting values from Godot, or should we just trust the engine with those invariants?

Considering this, I'm always asking myself: what do we lose if we validate?

Not directly related, but GDScript has the tendency to use questionable values to represent "invalid" states, since it doesn't support real error handling, e.g.

These are not violating invariants of the type itself, but may surprise users.

chitoyuu commented 1 year ago

The Rust docs currently call the Transform to represent an "affine transform", but Godot docs don't. In order to be affine, certain mathematical properties need to hold. I'm not sure if that's realistically enforcible, anyway -- but maybe it's just a documentation issue and the transform doesn't need to be affine.

I think the presence of affine_inverse(), judging from how its docs are worded, kind of implies that Transform can indeed contain non-affine transforms (emphasis mine):

Returns the inverse of the transform, under the assumption that the transformation is composed of rotation, scaling and translation.

If so, then it should be a documentation error on our side indeed.

if there are valid use cases to violate invariants -> they are not invariants, and thus don't need validation

Not directly related, but GDScript has the tendency to use questionable values to represent "invalid" states, since it doesn't support real error handling, e.g.

This is in fact the main concern of mine regarding the input validation problem: the overall API surface is huge, and there might just be some method that use the types in an undocumented way. In other words, it might be the situation that there's something we think is an invariant but is actually not.

Also related is the problem of floating point drift, where in a series of operations small errors add up and eventually become larger than EPSILON -- in this case we can cause panics in perfectly good code (in the sense that for humans it totally work), just because we decided to try to be clever.