Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions editor/import/audio_stream_import_settings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ void AudioStreamImportSettingsDialog::edit(const String &p_path, const String &p
beats_edit->set_value(beats);
beats_enabled->set_pressed(beats > 0);
loop->set_pressed(config_file->get_value("params", "loop", false));
loop_offset->set_value(config_file->get_value("params", "loop_offset", 0));
loop_offset->set_value(double(config_file->get_value("params", "loop_offset", 0)));
MJacred marked this conversation as resolved.
Show resolved Hide resolved
bar_beats_edit->set_value(config_file->get_value("params", "bar_beats", 4));

List<String> keys;
Expand Down Expand Up @@ -517,7 +517,7 @@ void AudioStreamImportSettingsDialog::_settings_changed() {

void AudioStreamImportSettingsDialog::_reimport() {
params["loop"] = loop->is_pressed();
params["loop_offset"] = loop_offset->get_value();
params["loop_offset"] = double(loop_offset->get_value());
params["bpm"] = bpm_enabled->is_pressed() ? double(bpm_edit->get_value()) : double(0);
params["beat_count"] = (bpm_enabled->is_pressed() && beats_enabled->is_pressed()) ? int(beats_edit->get_value()) : int(0);
params["bar_beats"] = (bpm_enabled->is_pressed()) ? int(bar_beats_edit->get_value()) : int(4);
Expand All @@ -543,7 +543,7 @@ AudioStreamImportSettingsDialog::AudioStreamImportSettingsDialog() {
loop_hb->add_child(memnew(Label(TTR("Offset:"))));
loop_offset = memnew(SpinBox);
loop_offset->set_max(10000);
loop_offset->set_step(0.001);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

loop_offset->set_disallow_step_rounding(true);
loop_offset->set_suffix("sec");
loop_offset->set_tooltip_text(TTR("Loop offset (from beginning). Note that if BPM is set, this setting will be ignored."));
loop_offset->connect("value_changed", callable_mp(this, &AudioStreamImportSettingsDialog::_settings_changed).unbind(1));
Expand Down
2 changes: 1 addition & 1 deletion modules/minimp3/audio_stream_mp3.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class AudioStreamMP3 : public AudioStream {
int channels = 1;
float length = 0.0;
bool loop = false;
float loop_offset = 0.0;
double loop_offset = 0.0;
void clear_data();

double bpm = 0;
Expand Down
2 changes: 1 addition & 1 deletion modules/minimp3/resource_importer_mp3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ Ref<AudioStreamMP3> ResourceImporterMP3::import_mp3(const String &p_path) {

Error ResourceImporterMP3::import(const String &p_source_file, const String &p_save_path, const HashMap<StringName, Variant> &p_options, List<String> *r_platform_variants, List<String> *r_gen_files, Variant *r_metadata) {
bool loop = p_options["loop"];
float loop_offset = p_options["loop_offset"];
double loop_offset = p_options["loop_offset"];
double bpm = p_options["bpm"];
float beat_count = p_options["beat_count"];
float bar_beats = p_options["bar_beats"];
Expand Down
7 changes: 6 additions & 1 deletion scene/gui/range.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ void Range::_set_value_no_signal(double p_val) {
return;
}

if (shared->step > 0) {
if (shared->step > 0 && !shared->skip_step_rounding) {
p_val = Math::round((p_val - shared->min) / shared->step) * shared->step + shared->min;
}

Expand Down Expand Up @@ -167,6 +167,10 @@ void Range::set_step(double p_step) {
shared->emit_changed("step");
}

void Range::set_disallow_step_rounding(bool disallow) {
shared->skip_step_rounding = disallow;
}

void Range::set_page(double p_page) {
double page_validated = CLAMP(p_page, 0, shared->max - shared->min);
if (shared->page == page_validated) {
Expand Down Expand Up @@ -258,6 +262,7 @@ void Range::unshare() {
nshared->max = shared->max;
nshared->val = shared->val;
nshared->step = shared->step;
nshared->skip_step_rounding = shared->skip_step_rounding;
nshared->page = shared->page;
nshared->exp_ratio = shared->exp_ratio;
nshared->allow_greater = shared->allow_greater;
Expand Down
2 changes: 2 additions & 0 deletions scene/gui/range.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class Range : public Control {
double min = 0.0;
double max = 100.0;
double step = 1.0;
bool skip_step_rounding = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)? 😅

Copy link
Contributor Author

@MJacred MJacred Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I appreciate it!

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

double page = 0.0;
bool exp_ratio = false;
bool allow_greater = false;
Expand Down Expand Up @@ -78,6 +79,7 @@ class Range : public Control {
void set_min(double p_min);
void set_max(double p_max);
void set_step(double p_step);
void set_disallow_step_rounding(bool disallow);
void set_page(double p_page);
void set_as_ratio(double p_value);

Expand Down
Loading