Stem Separation#572
Conversation
Implement V1 of the spec
Error handling for split fail Better logging
Separation is now handled for every caller. Max concurrent separations using a global.
RumovZ
left a comment
There was a problem hiding this comment.
- The table header is broken. (Could be an existing bug that you've triggered by adding new columns.)
- The buttons in the settings dialog are a bit confusing. No feedback is provided after selecting a (bad) provider, and it's not clear what
Infodoes. - Then there's another info button with an icon. This should have a dark variant.
- I think there should be an icon indicating the state of the selected separator? Seems to only partially work.
- The outer padding of the settings dialog has disappeared.
| )[0] | ||
| self.raise_() | ||
| if not filename: | ||
| notification.error("No file selected.") |
There was a problem hiding this comment.
I think in cases like these where a user actively cancels a dialog, no notification is needed.
I can't reproduce this. The header is fine for me, and @bohning didn't mention any issues either. Could this be something stateful, e.g depending on the saved geometry in the settings?
Could you elaborate? The icon and its tooltip should change based on separation provider state.
I don't disagree. I've spent quite a lot of time thinking about this, but can't come up with something I truly like. Do you have some general suggestions? The settings we need are:
|
I see. I can reproduce, but I don't really understand the issue (The header should be resetting, but it doesn't and tries to apply the old geometry). It fixes itself for me by just closing and reopening the syncer though.
In case you want to, check out https://github.com/bohning/usdb_syncer_separation , it's pretty simple. I fixed the error though. I also managed to get the icon to the top of the groupbox. I can't put any text in that label, the paddings would be way too big and interfere with the content above, but IMO the icon is okay.
It's shown in the "Provider Info" dialog. I put it there because we don't really have enough space to show a full-length path. I somehow broke the width of the settings dialog, so I'll have to see how that goes back, but otherwise, I'm quite happy now. |
|
Sorry for the delay. I didn't get around to review this thoroughly, but you seem to have addressed the most important stuff. If you want to merge, I won't stand in the way. |
|
I think what I'll do then is add a "(Beta)" text to the groupbox. I'm sure there are some problems that I'll just solve when they pop up. |
| vocals=Resource.from_nested_dict(dct["vocals"]), | ||
| instrumental=Resource.from_nested_dict(dct["instrumental"]), |
There was a problem hiding this comment.
| vocals=Resource.from_nested_dict(dct["vocals"]), | |
| instrumental=Resource.from_nested_dict(dct["instrumental"]), | |
| vocals=Resource.from_nested_dict(dct.get("vocals")), | |
| instrumental=Resource.from_nested_dict(dct.get("instrumental")), |
@randompersona1, this breaks reading any collections from before this change. Also, r-Q6wyggsTg.usdb must not be modified so we catch exactly this kind of breakage. I wonder how we can ensure that so we don't forget again in the future. 🤔


Closes #168
Much of the PR is based on or directly copied from #523 (thanks again, @bohning )
Also relevant is https://github.com/bohning/usdb_syncer_separation/blob/main/SPEC.md and https://github.com/bohning/usdb_syncer/wiki/Separation-Providers (both not finished entirely)
The primary architectural question was how to handle the separation binary: keep it open centrally in a global, or open it multiple times. I chose for every thread to have to open it separately to allow changing settings mid-download and having it handled like every other setting.
The current implementation deliberately trusts the separation binary to work. For example, there are no timeouts, little process killing, little error handling. I experimented a little with those things and plan to improve this in the future, but it would have caused bloat here and delayed the feature.
Another idea I had was a window to show stderr of the separation binary (difficult for downloads anyways, requires #552 ).
I didn't write a test for the JSON-RPC client because it is so simple, but I could if you want.