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

Add NAMFileBrowserControl to group Model/IR loading controls #242

Merged
merged 7 commits into from
Jun 10, 2023

Conversation

olilarkin
Copy link
Contributor

@olilarkin olilarkin commented May 6, 2023

This PR adds a new control NAMFileBrowserControl, which is used for the Model and IR loading. It

  • Groups all controls into a meta control
  • Implements browsing through adjacent files in the same directory as the chosen file with left/right arrows
  • Implements a pop-up menu with all .nam. or .wav files in the folder that the original file was from
  • Eliipsizes long file names that are displayed in the UI
  • Enables tooltips, and shows the full filename in the tooltip for the child control that displays the current model/ir name
  • Reworks the model-view communication to remove non realtime safe code - the previous approach was calling SendControlMsgFromDelegate() which you shouldn't do on the audio thread
NAMFileBrowseControl.mov

Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

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

All of the main changes are 10/10. Awesome to have them! And the metacontrol definition is awesome.

One critical issue that I'm not sure how to solve yet & I'd like to either have a fix or at least a plan before I can merge: The code in OnUIOpen doesn't seem to be executing anymore: when I reopen the UI, the model & IR text are back to their defaults "Select model..." etc. It looks like the code that affects what we see is now mFileNameButton->SetLabelStr()? Is there a way that this can be accessed from OnUIOpen?

As soon as either there's a fix or I understand how to clean it up, I'm good to merge this.

@olilarkin
Copy link
Contributor Author

All of the main changes are 10/10. Awesome to have them! And the metacontrol definition is awesome.

One critical issue that I'm not sure how to solve yet & I'd like to either have a fix or at least a plan before I can merge: The code in OnUIOpen doesn't seem to be executing anymore: when I reopen the UI, the model & IR text are back to their defaults "Select model..." etc. It looks like the code that affects what we see is now mFileNameButton->SetLabelStr()? Is there a way that this can be accessed from OnUIOpen?

As soon as either there's a fix or I understand how to clean it up, I'm good to merge this.

yeah i am aware that this is not working yet, this PR is a draft it's not quite ready to be merged

@sdatkinson
Copy link
Owner

this PR is a draft it's not quite ready to be merged

Right, apologies. Still an unintuitive feature to me (drafts that are visible to others) 😅

@olilarkin
Copy link
Contributor Author

Possibly blocked by #254

@sdatkinson
Copy link
Owner

Possibly blocked by #254

If we want to add the entitlement to ~/Music/NeuralAmpModeler just to demonstrate that it works, then that's fine and we can pick up the details (eg ensuring its existence) in a follow-up PR.

Just in case that helps manage scope to this PR.

@James-F2
Copy link

This glorious PR can't come fast enough. 😍
Windows explorer is slow as heck.

@towner96
Copy link

Thank you! Great advancement. Loading nam files will be faster now.

@jimc4893
Copy link

is this almost good to go?

@olilarkin olilarkin force-pushed the nam-file-browser-control branch 2 times, most recently from 082939a to 8ca8225 Compare May 29, 2023 09:17
@olilarkin olilarkin marked this pull request as ready for review May 29, 2023 09:20
@olilarkin olilarkin force-pushed the nam-file-browser-control branch 2 times, most recently from b76cf73 to 1a31ddb Compare May 29, 2023 10:04
@olilarkin olilarkin marked this pull request as draft May 29, 2023 10:19
@olilarkin olilarkin force-pushed the nam-file-browser-control branch 2 times, most recently from 341a00b to c7ace54 Compare May 29, 2023 12:35
@olilarkin olilarkin marked this pull request as ready for review May 29, 2023 13:02
@olilarkin
Copy link
Contributor Author

olilarkin commented May 29, 2023

@sdatkinson This is now ready for review. The model and IR path should be updated when the UI opens. There is a tonne of code clean-up around this and a bunch more UI rework that I would like to do, but one thing at a time I guess.

@olilarkin
Copy link
Contributor Author

I would suggest that you don't squash-merge the PR and make a merge commit for the PR.

@SoulReaverFan
Copy link

Looking forward to this one! Hope it’s coming in the next update 🙂

@sdatkinson
Copy link
Owner

Deleted the comments with invalid issues.

Please excuse me--I had the wrong commit on my local clone 😅

Windows is looking good. macOS looking good in the DAW; currently checking the standalone.

@sdatkinson
Copy link
Owner

sdatkinson commented Jun 10, 2023

Update looking into macOS.

There seems to be an issue with loading models and IRs. The behavior is that after clicking "Open" on the file selection dialog, the dialog closes, but the standalone continues with no model loaded.

The model is located at /Users/steve/Documents/NAM Models/model.nam.

Putting a breakpoint in ProcessBlock, I see that mStagedNAM is still nullptr.

Looking closer, I've identified that loadModelCompletionHandler isn't visited after clicking "Open".

First guess would be that this is related to the entitlements issue Oli called out in a separate Issue.

Continuing to look, will update.


Looks like ScanDirectory() is the culprit. I'm going to venture a guess that opendir() in d.First() is returning a null (edit: yep) because it can't be accessed (presumably bc missing entitlements.)

If that's the case, then I'll be happy to approve this PR and address the entitlements next as I said 🙂 . We'll want to get it squared up before the next release. Might be able to get it today or this weekend yet.

Copy link
Owner

@sdatkinson sdatkinson left a comment

Choose a reason for hiding this comment

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

This introduces a bug where laoding NAMs and IRs on the macOS standalone is broken. This is probably because of entitlements and is more or less anticipated in #254.

But we'll merge this and tackle that next.

@@ -207,6 +97,7 @@ NeuralAmpModeler::NeuralAmpModeler(const InstanceInfo& info)
pGraphics->AttachCornerResizer(EUIResizerMode::Scale, false);
pGraphics->AttachPanelBackground(COLOR_BLACK);
pGraphics->EnableMouseOver(true);
pGraphics->EnableTooltips(true);
Copy link
Owner

Choose a reason for hiding this comment

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

Cute!

@sdatkinson sdatkinson merged commit 4bddb7f into sdatkinson:main Jun 10, 2023
2 checks passed
@sdatkinson
Copy link
Owner

Update/Correction: Seems that Entitlements for the Music directory are already set.

Standalone works if the models are under the Music directory 😄 🎉

Question is how to inform users about this requirement. It will be a breaking change because users who have stored their models somewhere else will not be able to load them up at all anymore. Some modal would be great. I'll make an Issue for it.

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.

None yet

6 participants