Don't round loop_offset on import + Use double for loop_offset consistently

[MJacred]

Not sure if this PR should close #87427, but it would cover their requirement of a floating point value with the precision of 12 decimal places, as Godot's float provides for double 14 reliable decimal digits of precision. Except the value will be rounded/cut off again on saving to a file?

I removed the set_step call to prevent rounding, as we don't know how the user will use their given precision.

Side note: ogg vorbis was already using double consistently.

[MJacred]

Remark:
I think casting the result of get_value to double is unnecessary (as it returns double), but this line by Juan made me somewhat uncertain?

https://github.com/godotengine/godot/blob/e01fb324e5a6c8e5401b362a1954b9f692972cac/editor/import/audio_stream_import_settings.cpp#L521

[bs-mwoerner]

I don't have a compiler right now to check, but isn't the default step 1.0? If so, this would make it so that you can only loop on full seconds, wouldn't it?

[MJacred]

Indeed, https://docs.godotengine.org/en/stable/classes/class_range.html#class-range-property-step says default is 0.01. Let me check if I can do anything about this…

[MJacred]

The docs may be inaccurate, because in the code it says double step = 1.0; 😆

[bs-mwoerner]

Oh, right! That's definitely a documentation bug then. Looks like 1.0 has been the actual default value for years.

[RustyRoboticsBV]

Unfortunately, this doesn't resolve #87427. Removal of the step value causes the loop_offset field to only accept integer values.

After re-adding a step value (I used 0.00001), the old truncation problem still persists. For instance, 36.12318 becomes 36.1232 and 360.12318 becomes 360.123.

I think @bs-mwoerner is probably on to something here.

[MJacred]

@RustyRoboticsBV: I think I got it. If you want to test again?

[MJacred]

I'm uncertain if Shared is the correct place or the class Range itself. But I think it should be possible (at the very least internally), to prevent rounding.

[bs-mwoerner]

Sorry for the constant nagging without doing any actual work myself, but I think this new set_disallow_step_rounding() method has the same effect as simply doing set_step(0.0)? 😅

[MJacred]

It does have the same effect.

With the additional bonus of breaking the SpinBox: the increment/decrement will stop working.
With this change, it will keep working and increments/decrements in full seconds (which is not ideal, tbh, but first I want to see if it actually works, then we can talk about the steps. Or even use the current steps of 0.001).

If this works out, I might need to split the PR anyway, because I'm touching GUI stuff… (but first testing if it works)

[bs-mwoerner]

Ah, that's a good point! Yes, I didn't think about the step buttons. So this way, we could have a SpinBox that supports step increments but still allows floating point entries at full precision. Yes, I think this would be one way to work around the original problem of not being able to enter the precise "seconds" equivalent of the sample count.

[MJacred]

At first, I wanted to set negative steps to prevent auto-rounding. But then the buttons would be inversed 😆

[bs-mwoerner]

Yes, as I understand it, this is meant as an alternative to the set_step(0.00[...]1) approach (with the difference that one doesn't have to decide on a particular precision but can just order "max precision" for the returned value). It doesn't touch the serialization/deserialization issue. It would be interesting to know, though, whether the actual stream plays back correctly (with either approach). I think there the value is stored in binary and might retain the full double precision, so the loss may only affect a later re-import via the dialog.

[MJacred]

Bummer. Hm… I'll have a closer look over the weekend and ask in the core channel (sounds like the best fit because of the serializing)

[RustyRoboticsBV]

Thanks! I appreciate it!

In the meantime, I'm going to take a look at ustring.cpp.

[KoBeWi]

You can set separate arrow step for SpinBox. The new property is unnecessary.

[MJacred]

@KoBeWi: This PR will be changed (see #87882 (comment)). I haven't received any response yet, so I'll change the code as stated there.

And I completely forgot that we still run into rounding issues, even if double (de-)serialization would be fully exhausted. It just happens at a later decimal place. So rounding should happen either way.

[bs-mwoerner]

I've now explored the idea of having different units (Seconds/Samples/Beats) with different step values some more and created a separate PR for that #87882. That's definitely a more complicated solution than just tweaking the input field, but it allows using round numbers for samples and beats without having to convert them to lengthy decimal time values. This now also keeps the precision across reimports (although this is unrelated to how we handle the actual input). Maybe you can have a look at that and weigh in with some thoughts on whether the additional complexity (and exposing the sampling rate from the audio streams) is worth it.

[lyuma]

one reason I was pushing for this change is because it would be easier to keep compatibility with and doesn't require much consensus: it can be argued to be a bugfix, since the rounding we were doing was not a multiple of the sample rate and hence unpredictable.

Even if we do end up #87882, there might be reason to allow precise values in seconds (the user may still prefer to use seconds and copy the precise timestamp from their audio software), so it seems worth solving this in any case.

set_disallow_step_rounding or set_step(0.0) really ought to work when the native type is double, and if it's not working we should maybe see if there are bugs we can fix that are causing doubles to lose precision.

[MJacred]

@lyuma: see #87860 (comment). And yes, better double (de-)serializing should be done. But that is core. After that, we can increase the freedom of step (ie. making it way smaller) for loop offset further.