#4292 Add preference to disable submodel dup node check #4293
Conversation
|
A sub model of arms might have the first pixel in common or share a common
edge. Dups in sub models are very common.
On Tue, Jan 23, 2024, 10:23 PM Keith Westley ***@***.***>
wrote:
…
I disagree with this. While model groups it is hard to prevent duplicates
I can’t see any reason why you would do this in a submodel.
On 24 Jan 2024, at 11:36 am, Daryl ***@***.***> wrote:
Similar to preference to disable duplicate nodes in model groups.
Issue #4292<#4292>
________________________________
You can view, comment on, or merge this pull request online at:
#4293
Commit Summary
* fe753fc<
fe753fc>
#4292 Add preference to disable submodel dup node check
File Changes
(4 files<https://github.com/xLightsSequencer/xLights/pull/4293/files>)
* M xLights/preferences/CheckSequenceSettingsPanel.cpp<
https://github.com/xLightsSequencer/xLights/pull/4293/files#diff-2ea406a249e1fdf3e3a42bfdcec78d8ee73c559808d518d85abffe1aba50d6ed>
(15)
* M xLights/preferences/CheckSequenceSettingsPanel.h<
https://github.com/xLightsSequencer/xLights/pull/4293/files#diff-e81ee4891932d689589fa14999ad67e56ae37d561e068c9b35e9e34922e6f7c1>
(3)
* M xLights/wxsmith/CheckSequenceSettingsPanel.wxs<
https://github.com/xLightsSequencer/xLights/pull/4293/files#diff-aeed7f787551ef23a6cabf891141030496d2bc91f65a8f9bab176c2ea817cb94>
(11)
* M xLights/xLightsMain.cpp<
https://github.com/xLightsSequencer/xLights/pull/4293/files#diff-692facf05a050f4bde1e42db38106f55ee53ade6e8bfc97dc0903547dcd73c14>
(29)
Patch Links:
* https://github.com/xLightsSequencer/xLights/pull/4293.patch
* https://github.com/xLightsSequencer/xLights/pull/4293.diff
—
Reply to this email directly, view it on GitHub<
#4293>, or unsubscribe<
https://github.com/notifications/unsubscribe-auth/ACOOXKFNXOGNZOC7767H5PDYQBJPJAVCNFSM6AAAAABCH4POO6VHI2DSMVQWIX3LMV43ASLTON2WKOZSGA4TOMRQGU3DEMQ>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
—
Reply to this email directly, view it on GitHub
<#4293 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDNVKY56Q5SYKV4FHWQDWDYQB5ELAVCNFSM6AAAAABCH4POO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBXGI4TCNJQG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Here is mine for example .. |
|
Since this is now "protected" by an option and not on by default, the user(s) can easily choose which options works/fits best for them. |
|
@keithsw1111 are you still against this one going in? Looks like it has an option to disable/enable it. |
|
I am. I don’t think submodels should allow it without warning. Submodels are essentially custom buffers and duplicate nodes have no place in them. Unlike groups where the user can’t really do much about it there is no reason not to fix this.
On 26 Jan 2024, at 12:46 pm, Gil Jones ***@***.***> wrote:
@keithsw1111<https://github.com/keithsw1111> are you still against this one going in? Looks like it has an option to disable/enable it.
—
Reply to this email directly, view it on GitHub<#4293 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACOOXKCSFMMJXCZTZG3ZKQLYQMDIPAVCNFSM6AAAAABCH4POO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJRGI3TQNJWGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Ok maybe I was confused on what we were saying here. So your saying inside a single submodel there should be no duplicate nodes. That’s sounds reasonable. I thought we were talking about the same node in different submodels for the same parent model. Guess I need to hear why duplicate nodes in the same submodel are a common thing. |
|
A single submodel .. made up where each line makes up an arm in a spinner will show duplicate nodes if they share a single pixel in the center. A single arm - sure .. no duplicates - but that gets lost in this duplicates. Perhaps the dup node check needs a little tweak and allow duplicate nodes in the various lines. With this change - a user can go thru their list, get the dups and then can ignore the rest. |
|
Kind of wonder if this could/should be a submodel specific setting. "Acknowledge duplicate nodes" kind of thing. |
|
With this change it is. The current one allows you to ignore duplicates at the group level, but this one was to allow at the sub-model level. As @derwin12 indicated, this is realy not a duplicate per-say. More and more folks are using submodels to create "stacked"/multi-line sub-models so the duplicates are across multiple, indvidual rows and not on the same row. |
|
No, this change makes it a GLOBAL setting affecting all submodels. I'm suggesting make it specific to EACH submodel and saved in the rgbeffects as part of the submodel. Thus, if you do introduce duplicate nodes, you will still get a warning OR you would need to acknowledge for that model that it is intentional. |
|
It's funny, I have 1200 duplicate node in submodel warnings in my layout and not a single person out of the thousands of viewers has ever come up to me and said, "Hey MoC, you know your show would be a lot better if you spent a few hours cleaning up the duplicate nodes in your submodels". I have actually noticed that I have a duplicate node, but only once out of 14 hours of material I ran last year. It's quite common in stranded submodels. Here's a list of the 95 models (out of 1136) that have submodels with duplicate nodes provided by the vendor: While I'm on the soapbox, a 27 vendor models also have submodels with node numbers outside the range of the model: Is it really so wrong to want to suppress this one warning, so you can see all the useful stuff check sequence has to offer? I'll be including the patch in the builds I make for myself... thanks for providing it @derwin12. |
|
Really no excuse to have them in the same sub model. If it’s the center pixel of an arm then take that one out before you duplicate it 20 times then just have one line with the center pixel by itself. It’s one sub model it’s not like we are lighting up those rows individually |
But you do .. single strand goes around and lights up each petal/spoke etc .. That's why dups are needed. |
|
Ok I see what you mean. I’m not gonna mind either way I never run check sequence |
|
Right.. this is only an "issue"/annoyance... since some of the coro vendors are doing proper submodeling....
|
|
Perhaps if I create another ticket to for an additional check sequence to
flag duplicate nodes on line items. Not sure how to term that.
That should indeed be cleaned up.
Like eg.
1-10,5,6,5
…On Sat, Jan 27, 2024 at 5:51 PM AlexB ***@***.***> wrote:
Right.. this is only an "issue"/annoyance... since some of the coro
vendors are doing proper submodeling....
They will not be duplicate of they were split, actually they already are
but there's also a Join one.
image.png (view on web)
<https://github.com/xLightsSequencer/xLights/assets/23619433/ac04f33f-c96c-4ebd-a284-67497450f60a>
—
Reply to this email directly, view it on GitHub
<#4293 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDNVK4RKK37BKJVXPH2NWDYQWAIHAVCNFSM6AAAAABCH4POO6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJTGM2TQMZRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Been waiting for this. Any reason why this can't be merged... please.... |
As mentioned, this PR puts this as a global level setting which is not appropriate for it. It needs to be redone as a submodel level checkbox on the submodel dialog. |
|
I still dont get it. I dont mind a node appearing in different submodels but it does not belong in a single submodel. It does not do what you think it does and the user should be aware of that. The spinner example is a rubbish example. You are thinking the node is duplicated on each line ... and it isnt ... only one of those instances actually works ... all the others get silently removed. This is not acceptable. This is fundamentally wrong and the sooner this is closed the better. |
|
Closing this PR as it's not how it's needed. |
|
Agreed. Replaced by: #4316 |
Similar to preference to disable duplicate nodes in model groups.
Issue #4292