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

Allow for changing of the View when importing a sequence #4364

Open
kevinsaucier opened this issue Feb 23, 2024 · 22 comments
Open

Allow for changing of the View when importing a sequence #4364

kevinsaucier opened this issue Feb 23, 2024 · 22 comments

Comments

@kevinsaucier
Copy link

kevinsaucier commented Feb 23, 2024

I think this is more of a bug than a feature request but it's technically working as designed (I think) so figured I'd make it a request.

When opening an existing sequence, if you're missing a bunch of models, you're prompted to import instead of opening. This is great. Unfortunately, when you get to the import dialog, it appears to only contain the models/groups from the sequence and not from the current available views and you have to close the dialog, change the View, and reopen the import dialog again to finish mapping.

Prompted and click No

image

Import dialog opens with what appears to be a random sort (ideally, this would be fixed to Alpha sort as well) so I resort it by model name into:

image

You can see here that Auto Map was not particularly helpful, even though I have numerous aliases set up, because the matching props/groups are not in the list.

image

Cancelled the import dialog and made my preferred view (set in Preferences) my Master, then opened the import dialog again. Again, the list seems to be sorted in some random fashion so I re-sort it by name into:

image

You can see AutoMap was far more successful this time as the matches were available. Also, there are far more models in the 'Available' column than the first time. I'm assuming the first time only shows 'unmatched' props and this time, of course, is showing all props. I don't see a way to open the list of only 'unmatched' props again after changing the view. I guess it's not a big deal, as AutoMap continues to map them again, but it's a bit confusing.

image

What I would expect to see: When you click No to the 'Do you want to continue opening this sequence' dialog, it should then ask which View you want to set as Master for the sequence prior to opening the Import window. It could also just default to whatever view is set in the Preferences as the Default View. I know that would probably result in the View of the sequence changing in the background but, since the sequence shouldn't be open yet, that shouldn't really cause any issues because no effects should exist yet.

Also, after clicking No, it appears that it still 'continues opening the sequence' anyway as it's still mapping effects to props behind the import window. This behavior is also confusing. Clicking No should result in the full Import dialog opening and no effects being mapped unless they are imported.

Lastly, though loosely related, since the list of Models in the Import dialog doesn't seem to have any reasonable sort applied, the grid should default to being sorted by model/group name (or by the sort of the applied View, which makes more sense to me, but may not be the popular choice.)

Thanks!

@kevinsaucier
Copy link
Author

kevinsaucier commented Feb 23, 2024

Yikes, even more fun now, xLights is crashing every time I try to render the mapping I screenshotted above. 😬🤦‍♂️

I only got the crash dump the first time, which was right after I took the screenshots above. Dump files attached. I've now gone through a couple more times the 'clean' way....Open, click No, close Import Dialog, set View to Master, Open Import Dialog, AutoMap, Close Import Dialog, Render. 3 crashes with no crash dump dialog.

xLights_dbgrpt-143948-20240222T193535.zip

@cybercop23
Copy link
Collaborator

The sort order is a side-effect of this change #4278. There's also a new PR to allow expand/collapse all, and I thought it also added sort. We'll revisit the sort.
For the crash, that's been fixed in the nightly, if you want to download that and test it to make sure it no longer crashes.

@kevinsaucier
Copy link
Author

Thanks. Yup, sounds like just showing the Master view (assuming the master view is correct) would be the easy answer to sort in a meaningful way while still keeping the organization of the submodels. Though, in the long term, it would make more sense to have the ability to create groups within the submodel 'tree' instead of the hack that is being done to organize submodels now.

Thanks for the quick reply!

@kevinsaucier
Copy link
Author

For the crash, that's been fixed in the nightly, if you want to download that and test it to make sure it no longer crashes.

Yup, the nightly build fixed the crash. Worked the first time. Thanks!

@cybercop23
Copy link
Collaborator

For now, although not clear enough, you can click on the Model column to sort it up or down, but that also sorts the sub-models which we were trying not to do.

@kevinsaucier
Copy link
Author

For now, although not clear enough, you can click on the Model column to sort it up or down, but that also sorts the sub-models which we were trying not to do.

Thanks. Yup, I know how to sort it (it's sorted in my screenshots), it was just an extra step that seemed like it could be automated (though I understand why it's not). Hopefully, you can come up with a way to apply some useful order without 'messing up' the submodel ordering. My vote is to show the models in the same order they are listed in the current (which should be Master) view, as I think that makes more sense when mapping. You could also do like some applications do and intercept the click on the Model column to give sort options (Sort Alpha, Sort by View, etc.), though a lot of the user base is probably not used to the multiple sort ability and would be confused. :)

@cybercop23
Copy link
Collaborator

We've been discussing this for the last 2 hours in the Zoom room. What you see NOW is the Master Order View, but each group automatically gets expanded and it shows the group models. We're toying with the idea to NOT expand the groups and show you ONLY what's in the Master View. 5 items in master view... 5 items in the import list. This will provide a cleaner view of exactly how effects are placed and the render order.
We're also looking to make the fom (right) side - have the original sequences view order (still be able to sort if needed) but that will too provide a better understanding of the render order.

@kevinsaucier
Copy link
Author

Thanks for the update. Not trying to beat a horse, just making sure my original request is clear.

So, that makes a little more sense, but doesn't seem to be exactly what I'm seeing on the Import Dialog that comes up after clicking No (the original issue on this ticket). For that window, you can see here that I have a couple of groups at the top. The first is not expanded. The next few are is, though they don't show the arrow to the left of the group name for some reason. However, those groups are not in the order of my Master View.

image

Here, if I cancel the dialog, you can see the Master view has a different list of props.

image

And here is the list of props after using my 2024 view and setting it to Master:

image

@kevinsaucier
Copy link
Author

kevinsaucier commented Feb 23, 2024

Ahhhhhh, alright, so just realized/remembered that I have a few groups within that first group that doesn't look to have been populated. So that's why it looks like it didn't work, because the first 'All' group is expanded and has the next 5 'All' groups within it. That's where those 'expanded' arrows would really be handy, not to mention a full fledged tree control. 😁

@kevinsaucier
Copy link
Author

Alright, I'm back again..... 😬

So, looks like, on the Import Dialog, once the model has been shown in one group, further groups down the line don't expand to show that model again. This makes sense, conceptually, but doesn't really work from a UI standpoint.

You can see here that All Blinders shows the 20 blinders but then none of the other Blinder groups show anything.

image

I definitely vote to change this default expansion as it doesn't really make any sense during importing and makes things hard to find. Also, the fact that it shows the models within the group under the group, as if it had been expanded, but the group itself cannot be expanded/contracted just doesn't work. I vote to either sort everything alpha or sort by the Master View with groups not expanded.

The only issue I see with the 2nd option is that I have props that 'only' exist in my Master view within a group, not individually. For instance, I don't have all 20 blinders individually in my view as I just expand the All Blinders and add an effect to a single blinder if I need to. No need to take up all that space for nothing. It looks like they get added to the import dialog because they exist within a group that /does/ exist in my Master view, and then added to the Master model list when you click OK, but just wanted to point out the potential for an issue if you're making changes.

Thanks again!

@cybercop23
Copy link
Collaborator

RIGHT.. so the fact that they only expand from the first group is misleading. When they were sorted, you couldn't tell that, but having MV order and only show the same items makes the most sense.
So we're making it not to expand at all. There's no (yet) support to expand a group like you can a model, so the left side list will be exactly what you see in Master view, w/o expanding any groups. If you want to put something on an inner item of a group, you need to add it to the master view (as it should be - to enforce render order).

@kevinsaucier
Copy link
Author

Gotcha. But, by adding an effect to a model within a group when that model doesn't exist elsewhere on the layout, doesn't it just get rendered using the rules governing group rendering (render the group first and render individual props within the group starting from the bottom up)?

@kevinsaucier
Copy link
Author

RIGHT.. so the fact that they only expand from the first group is misleading. When they were sorted, you couldn't tell that, but having MV order and only show the same items makes the most sense. So we're making it not to expand at all. There's no (yet) support to expand a group like you can a model, so the left side list will be exactly what you see in Master view, w/o expanding any groups. If you want to put something on an inner item of a group, you need to add it to the master view (as it should be - to enforce render order).

Also, I don't want to get off topic from the original issue, which is that the 'current' master view is not used after clicking No on the 'Do you want to continue opening this.....' pop up. It's far more work to have to create a new sequence, go out and pick the audio file, set the FPS, then change the view to Master, then open the Import Effects dialog and go find the sequence file than it would be to just have the dialog default the 'Master' view to use the 'Default View For New Sequences' and automatically make it Master.

If your proposal will work in this instance as well, great. If not, that's actually the issue I'm looking to have resolved as it's going to be very time consuming during my 2024 mapping otherwise. 😁👍

image

@cybercop23
Copy link
Collaborator

Yes, 2 diffent issues.. that's what happens when we get "also.... double also...." :)

@kevinsaucier
Copy link
Author

Yes, 2 diffent issues.. that's what happens when we get "also.... double also...." :)

LOL....I know.....I know.....and I chastise my user base for doing the same thing, especially when one 'also' has to go to one group and the other 'also' has to go to another. 🤦‍♂️ I can open another ticket 'also', if you'd like, and it sounds like you already have the 'Lastly' figured out so this one is mostly on the original problem. 😁

@derwin12
Copy link
Collaborator

I am tagging a Pull Request to this .. sounds like you reverse engingneered what is happening. The goal here is to put the Master View onto the import mapping screen with a big caveat here .. IF you don't have the individual model in the master view it will NOT be shown on the import screen. If you want it to show - then use a slightly different master view just for the mapping phase.
There is no way to include the same model multiple times on the import screen. As for your master view - if you include an effect on the expanded group it "silently" adds the prop on the bottom of your master view.

@kevinsaucier
Copy link
Author

@derwin12 So, right now it seems to show the models if they exist within any group in the current view, regardless of whether or not they exist individually. They will then be added, as you said, silently to the end of the list on the master view.

If I'm understanding you correctly, this behavior will be changed to where you can no longer see those same individual props, which seems like a big change in behavior. I know everyone 'loves' options 😬 but would it be feasible to add a checkbox for 'show all models' vs 'show master view' or something to that effect? Maintaining a completely separate view just for imports doesn't seem user friendly.

@derwin12
Copy link
Collaborator

derwin12 commented Feb 23, 2024

That's a tough call .. then you get the "mess" you have now .. where items are sprinkled in amongst the groups. Wouldn't it be cleaner just to use your master view? This way you control how it shows .. all based on the master view. The list of models is loaded BEFORE it shows the dialog box - so adding a check box want really work. (Luckily its only February so we can tweak things -- its only code lol)

@cybercop23
Copy link
Collaborator

If we make it a preference option, it will apply to all imports, so you can choose old/current or new way. It is a "big" change, but until we took it all apart today, I don't think a lot of people realize how it actually works and the fact that if you do put an effect on an itme in a group, it adds that model to the MV and hides it.

@kevinsaucier
Copy link
Author

Maybe I'm confused. As it stands today, it seems like it's loading the groups in order of the Master view, then expanding the groups starting at the top but only showing models that weren't part of a group that's already been expanded. That seems to be what my screenshots above show. If that's the case, couldn't the 'mess' be fixed by simply alpha sorting that list? Then, you'd basically be getting 'every' device that you care about (either added individually or added to a group that's in your Master) . If something is then mapped to a model that doesn't exist in your Master view, it could be added to the bottom of the list, just as it does now (though I wouldn't default it to be hidden, as it does now).

Once of my skills is coming up with crazy crap and getting my programmers to figure out how to do it. 😁 My thinking on the check box is you could add 'all' models/groups into memory, then apply the filter when you load the window. Clicking the 'Show All' would then simply remove the filter and refresh the grid with what's already in memory.

@kevinsaucier
Copy link
Author

If we make it a preference option, it will apply to all imports, so you can choose old/current or new way. It is a "big" change, but until we took it all apart today, I don't think a lot of people realize how it actually works and the fact that if you do put an effect on an itme in a group, it adds that model to the MV and hides it.

Glad I could give everyone something to do today. 🤣

At some point, I need to work on setting up a development environment as I'd really love to be able to help, though it's been a bit since I've worked in C so I'd have to do some refreshing. 😬

@cybercop23
Copy link
Collaborator

The also part has been implemented. Will be available in the next release or tonight's nightly build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants