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.

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

Project export window fixes #7724

Merged
merged 3 commits into from
Mar 2, 2025
Merged

Project export window fixes #7724

merged 3 commits into from
Mar 2, 2025

Conversation

zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Feb 20, 2025

Two small changes.
Edit: Only one fix left here. The increased number of loops on export.

1 - Fix a glitch in the project export window. Glitch visible with for instance FLAC export.
2 - Increase number of max loops when exporting to 999. I need more. You also need more.

Edit: This issue was on my side and related to os selected screen size.
Buggy file format settings window
exportwindow

Fixed
exportwindowfixed

@szeli1
Copy link
Contributor

szeli1 commented Feb 22, 2025

I have tested this PR and found that the window sizes work correctly on my system (linux mint, 4k display but full HD resolution used)

Before: (master)
pr_7724_1

After: (7724)
pr_7724_2

I'm not sure what is causing this, but it should be looked into, so I don't really want to approve

The code changes look fine

@zonkmachine
Copy link
Member Author

I'm not sure what is causing this, but it should be looked into, so I don't really want to approve

Interesting! I dived into my screen setup to see if I had done anything sketchy and as it turns out there was something that popped up. I had the screen set to max resolution which is higher than the real on board screen. So the graphic card and Qt has been working extra hours to keep up with my demands and reinterpret the screen. The funny thing is that the only glitch I've found so far was that one window in LMMS. Nothing else. I solved it on my side by setting the resolution to the built-in screen and that will work on my side. Considering that no other window glitches on the laptop with wrong setting, maybe there is a better way to draw that window, but I think for now the problem is on my side. If there is something that needs fixing it's for someone else.

Screen Resolution 	‎1366 x 768 pixels
Max Screen Resolution 	‎1920 x 1080 Pixels 

Thanks for the review!

@zonkmachine
Copy link
Member Author

Reopened for the loop export fix.

@tresf
Copy link
Member

tresf commented Feb 27, 2025

This is an odd PR because the history for it says a lot more than what it does. What I can read from the diff is that someone wanted to have segments looped more than 99 times. I'm not sure where this value was chosen from and I'm also not sure where 999 comes from. I see no reason to make this anything less than what the underlying model value supports. That said, no objections.

@zonkmachine
Copy link
Member Author

This is an odd PR because the history for it says a lot more than what it does.

Yes. It got messy as the original start of this PR was invalid and I choose to force push. Messy. Tempting to make a separate new PR instead.

Loop length

At 99 maximum loops a one measure beats exported length is 3m 18s.
Increasing the number of loops to 999 will allow a beat to loop for 33m 17s.
Half an hour is not long enough for a soundscape but you could quite easily extend the loop and re-export. But I wonder why there is a maximum in the first place. If a user want's to loop a track for a very long time maybe there shouldn't be an upper limit or it should be longer. 9999?
According to my friend Grok, wav file tap out at almost seven hours.

So, for a standard WAV file at 44.1 kHz, 16-bit, stereo, the maximum sample length is about 6.75 hours.

When I export a 1 measure loop 9999 times at 120 bpm it comes out at 3h 22m 53s.
I think this sounds better actually. It would allow the user to experiment more and it won't glitch out at those export lengths.

Should I open a new PR?

@tresf
Copy link
Member

tresf commented Mar 2, 2025

I wonder why there is a maximum in the first place

Agreed.

tap out at almost seven hours.

Right because 4GB size limit, but that's only WAV. Other output formats would behave differently. I don't see a reason to set a limit.

Should I open a new PR?

Maybe I misunderstand.... What's wrong with this one?

@zonkmachine
Copy link
Member Author

Should I open a new PR?

Maybe I misunderstand.... What's wrong with this one?

As per this comment:

This is an odd PR because the history for it says a lot more than what it does.

I can make a cleaner one.

@tresf
Copy link
Member

tresf commented Mar 2, 2025

Should I open a new PR?

Maybe I misunderstand.... What's wrong with this one?

As per this comment:

This is an odd PR because the history for it says a lot more than what it does.

I can make a cleaner one.

Oh! No need for a new PR, it was more of a casual observation.

@zonkmachine
Copy link
Member Author

I bumped the value to 9999 and exported an organ loop with random arpeggiator elements, filter sweep, and delay/reverb, over some five hours in both wav and ogg and it was glorious.

I had a go at removing the loop count maximum completely but it would default to 99 anyway. It looks like some default value in Qt or at least not as something I can do anything about. Just bumping the maximum number of loops seem to me as an acceptable tradeoff.

@tresf
Copy link
Member

tresf commented Mar 2, 2025

If there's nothing preventing it, should the max be the underlying model max? (e.g INT_MAX, UINT_MAX or whatever?)

@zonkmachine
Copy link
Member Author

it would default to 99 anyway. It looks like some default value in Qt or at least not as something I can do anything about.

Yes, the default max for a QSpinBox in Qt Designer is indeed 99 (arbitrary default set by the tool). Same as our old max.

If there's nothing preventing it, should the max be the underlying model max? (e.g INT_MAX, UINT_MAX or whatever?)

You would need to do that from on the c++ side and not in the Qt .ui file (xml) it's going to increase the complexity. This means that someone else need to fix this (which I'm perfectly fine with). It sounds like quite a bit of an overkill since now you can loop it for some fiver hours and if you need more, and off course there is people out there that do, there's always someone..., but in that case you can extend the pattern to be longer than one measure. We used to be limited to ~3 minutes and now it's ~6 or so hours. Good enough for me. ;)

@tresf
Copy link
Member

tresf commented Mar 2, 2025

You would need to do that from on the c++ side and not in the Qt .ui file (xml) it's going to increase the complexity.

No, sorry, I didn't mean to suggest that we calculate it from C++, I meant to steal a sane value from C++. For example, if unsigned integer on 32-bit is a baseline, perhaps a value of 4294967295 because it goes from arbitrary/magic to justifiable. That's all.

@zonkmachine
Copy link
Member Author

For example, if unsigned integer on 32-bit is a baseline, perhaps a value of 4294967295 because it goes from arbitrary/magic to justifiable. That's all.

OK. Grok says:

Default max in Qt Designer: 99 (arbitrary default set by the tool).

Absolute max for QSpinBox: 2147483647 (limited by int).

You can set any value up to 2147483647 in the .ui file or programmatically.

For larger values, switch to QDoubleSpinBox.

So...

spinboxlargemax

Sure about this... 🥲

@tresf
Copy link
Member

tresf commented Mar 2, 2025

We may not have enough time in our life to test out this value, but it makes sense (mathematically) works and doesn't crash, merging.

@zonkmachine
Copy link
Member Author

merging

K. I'll Squash/merge after the tests have run.

@tresf
Copy link
Member

tresf commented Mar 2, 2025

merging

K. I'll Squash/merge after the tests have run.

Oh, I'll hold off and let you do it then... almost... pressed.... it.

@tresf tresf self-requested a review March 2, 2025 18:55
@zonkmachine zonkmachine merged commit ef1d86f into LMMS:master Mar 2, 2025
10 checks passed
@zonkmachine
Copy link
Member Author

Oh, I'll hold off and let you do it then... almost... pressed.... it.

Perfectly fine to just merge...

@zonkmachine zonkmachine deleted the exportgui branch March 2, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants