xLightsSequencer / xLights

xLights is a sequencer for Lights. xLights has usb and E1.31 drivers. You can create sequences in this object oriented program. You can create playlists, schedule them, test your hardware, convert between different sequencers.
GNU General Public License v3.0
530 stars 197 forks source link

2024.12 and Nightly crashing on layout center group select/right click #4633

Open derwin12 opened 1 week ago

derwin12 commented 1 week ago

Shown last night to Scott .. xLightsProblemCrash.zip

Open layout, Click on Wreath Rings .. on the layout screen right click and "Set Center Offset Here" .. do it like 3 times in various places. Now click on subs and then right click and "Set Center Offset Here". Repeat once more and crash.

Walking from an exception.
[01]    ADDR:0x00007ff784787a85 HMODULE:0x00007ff783fd0000  00000000007b7a40    LayoutPanel.obj ?ModelsSelectedCount@LayoutPanel@@IEBAHXZ@562685 
[02]    ADDR:0x00007ff78477aecc HMODULE:0x00007ff783fd0000  00000000007aad80    LayoutPanel.obj ?OnPreviewRightDown@LayoutPanel@@QEAAXAEAVwxMouseEvent@@@Z@555ACC 
AzGilrock commented 1 week ago

It the new AI. We detected you were having a seizure so we shutdown the program. I mean seriously if you gotta work that hard to crash it I kinda don't care to fix it.

AzGilrock commented 1 week ago

And what someone should really look at is why is the program doing a RecalcStartChannels that many times in 5 seconds and all the [ERROR| Really strange.... comments in the log.

derwin12 commented 1 week ago

And what someone should really look at is why is the program doing a RecalcStartChannels that many times in 5 seconds and all the [ERROR| Really strange.... comments in the log.

And that might be the trigger .. stuff is going on in parallel that needs some sort of block to stop it from happening.

cybercop23 commented 1 week ago

I know right.. we really need to dig into the recalc. Shouldn't be doing it so much, but I guess it was always just rest to add doasap work and let it ride.

AzGilrock commented 1 week ago

Yeah I can't search the code right now and I'm responding by email cause the darn two-factor authentication prevents me from getting logged in without a pain in the butt effort. I can't have my cellphone with me at work so doing the 2FA sucks.

I'm really curious why RecalcStartChannels is even running if he was just right-clicking to set a center-point. Even if it was just selecting a model that shouldn't be triggering that.

derwin12 commented 1 week ago

all the [ERROR| Really strange.... comments in the log. I believe there is one for each of the submodels in the group. So it is flagging/erroring the group containing a submodel rather than an actual model.

derwin12 commented 1 week ago

Doing some debugging .. this might not help anyone :-) If I click on the wreaths and right click .. it correctly says .. hey no models here .. just 10 submodels. When I right click again .. it (apparently) doesnt know what is selected, perhaps because of the reload/recalc. Anyways.. not with sees all of the models in the preview - not just the wreath that was selected. ie. It now thinks all the models are selected in the preview window not just the single item/wreath.

derwin12 commented 1 week ago

This guy ... scary.. It is messing up the model chosen etc .. when the group is all submodels.

xlights->GetOutputModelManager()->AddASAPWork(OutputModelManager::WORK_RELOAD_ALLMODELS, "LayoutPanel::OnModelsPopup::ID_SET_CENTER_OFFSET", nullptr, nullptr, GetSelectedModelName());

AzGilrock commented 1 week ago

Pretty sure there was something that was not updating on the screen without that line. I don't add them unless I'm fixing an issue. I bet there would be no issue if we forced that to finish before letting to try to set the center again... It's the back to back to back operation causing this.

computergeek1507 commented 1 week ago

Is it broken in .11 and what happens if you revert Daryl's center point changes?

derwin12 commented 1 week ago

That code is indeed required .. or it doesnt update the x & y positions. The code that is causing it .. is this one that I flagged a while back. It adds submodels to the models list. It causes a bunch of assert exceptions. It seems to be fine without the push, ie just ignore submodels when building the list of models. Thoughts on this - do I push the pr to do some more testing? (Line 1521 layoutPanel.cpp) This was likely there for a long time but really only shows a problem with groups of entirely just submodels.

                        if (m->DisplayAs == "SubModel") {
                            if (mark_selected) {
                              //  prev_models.push_back(m);  // setting this causes exception when prev_models render finds a submodel
                            }
                        }
derwin12 commented 1 week ago

Also that code makes it go here.. image

blun23 commented 1 week ago

Was this code added because you couldn't click on a group with only submodels in it and set center point? That was fixed a month or so back. If you clicked on a group of models or a group that had both submodels and models in it, you could right click and set center point. I don't know all the code stuff (yet, we will see if I get bored and deep dive), but there has to be something along those lines.

derwin12 commented 1 week ago

No, this was here years ago. The center point just focused and highlighted this crash during testing and revealed the bug. ie Groups of submodels. Seems it was always there it perhaps was just masked by things like the above. Not 100% certain though .. just a novice's view.

AzGilrock commented 1 week ago

Yeah from my brief look there was a lot of code added/changed for the submodel stuff around 2020 that looks questionable. That code wasn't done by one of the core developers and I see things that got commented out saying they weren't needed and I'm like yeah he had no idea why we even set things like that. There's a lot of times when writing code you will add a simple line like setting the "group highlighted" flag that causes code in a completing different file to work different and then someone comes along and doesn't understand why its there and deletes it.

AzGilrock commented 1 week ago

Wish I had more time to dig into recent issues but its getting so close to Christmas Expo that I've really got to use my spare time to make sure I'm ready for the classes I need to teach.