Scalar fields now rely on an internal offset to properly manage double/large values #116
Conversation
…e values (as long as the min and max values are not too far from each other!)
| using std::vector<float>::shrink_to_fit; | ||
| //! Shortcut to the (protected) std::vector::empty() method | ||
| using std::vector<float>::empty; | ||
|
|
There was a problem hiding this comment.
Should also add one for resize
There was a problem hiding this comment.
I prefer/want the user to use 'resizeSafe', especially if a default value is provided for new elements (the offset wouldn't be taken into account with the resize function input - which would also be casted to float)
There was a problem hiding this comment.
And I agree about making the std::vector as a member, I hesitated quite a lot.
There was a problem hiding this comment.
Got it for resizeSafe,
I would personally put std::vector as a member, but it has worked for CC for years so its not that bit of a deal I think
| inline void setValue(std::size_t index, ScalarType value) | ||
| { | ||
| if (m_offsetHasBeenSet) | ||
| { | ||
| // use the already set offset | ||
| (*this)[index] = static_cast<float>(value - m_offset); | ||
| } | ||
| else if (std::isfinite(value)) | ||
| { | ||
| // if the offset has not been set yet, we use the | ||
| // first finite value as offset by default | ||
| setOffset(value); | ||
| (*this)[index] = 0.0f; | ||
| } | ||
| else | ||
| { | ||
| // we can't set an offset | ||
| (*this)[index] = static_cast<float>(value); // NaN/-inf/+inf should be maintained | ||
| } | ||
| } |
There was a problem hiding this comment.
Hum I wonder if this is ok in the general case:
If ScalarType is float and I have some big value stored on a double, calling setValue will first cast my double into a float, so the loss of precision already happenned, its too late to apply the offset
The same thing goes for getValue which returns a ScalarType
Unlesss scalar type is no longer meant to be a float
There was a problem hiding this comment.
Indeed, ScalarType is double and should not be changed anymore (this would kill the purpose of all this code). I don't think we need to support the float case.
There was a problem hiding this comment.
In that case I think ScalarType should be removed (replaced with double) globally, or at least in these function, so its explicit that only double are expected
…e strange things when setting the first non-zero value later!
No description provided.