pac3 icon indicating copy to clipboard operation
pac3 copied to clipboard

Events using affect children only + target part no longer work

Open TheReeb opened this issue 1 year ago • 7 comments

So as the title describes itself, A few weeks ago (Around 2nd October) PAC was updated prior to fix force dampening. However, around the time it was updated it also made events that use affect children only + target part stop working entirely. While I'm not sure why this change needed to happen, here is why it should be reverted:

  • It allows the user to construct organized event bundles within a group, making it easier to read and quickly setup things. Now it requires us to put target part events within separate groups to achieve the same result, which is unnecessary.

  • Events working with "affect children only" ticked and using target part has been a thing for perhaps years.

  • It introduces alot of unnecessary work to fix existing PACs that worked perfectly fine, before this change was made. I've attached an image that show one use case of this event setup, that no longer work.

(The lightning should not be appearing and the green lasers go off, when they shouldn't.) Screenshot_5

Please consider reverting this change, as its more inconvenient than necessary. Branch used: PAC-develop Server: Buildstruct

TheReeb avatar Oct 12 '24 01:10 TheReeb

How bothersome.

Consistency. With proxies, affect children cancels out target part. No one complains about that. The wiring should be the same so that people know what to expect when they use the wiring methods. Even more so, when it says affect children only, it means affect children ONLY. Hello?? Do the words mean nothing at all??

I can't believe you in good faith. The event you highlight (the timerx) in your example doesn't even have children, there's only a target part. You expect me to believe it was a bad decision to make affect children only do what it says?

But I know why it happened. target part wasn't isolated properly before, it affected the parent and that was wrong, until I fixed that. especially when timerx, specifically, is very fickle (if it affects its parent, it'll hide itself preventing it from working properly). It's important that events isolate properly what they should do. I get that. No one put that into question. That's why target part now properly ignores its parent so you don't need to resort to affect children only to make it obey your downward structure. But at some point I have no qualms about forcing people to do things the way it's intended to. If it says affect children only, ONLY is an important word. The warnings will stay there out of spite to remind you not to use ACO if there are no children.

Events working with "affect children only" ticked and using target part has been a thing for perhaps years.

No it has not been "years". Affect children only functionality has not changed. target part for events is kinda new. It's not even on master yet. And let's not equivocate here. The old, pre-develop variable "target part" is the biggest misnomer and is not what we're talking about. It's downright criminal, which is why I renamed it "external origin part" in the editor friendly line, because it denotes PHYSICAL ORIGINS for detection, not event wiring. The actual target part we wanted all along is internally called DestinationPart to avoid confusion, but for users its editor friendly name is "targeted part". The more you know...

I have considered. All there is to it is that I can't fight the tide. I'll revert the change and make each wiring method mostly independent. That too has some issues but I'm not turning event code into a maze of exceptions in an effort to minimize the changes. There should be one design principle that makes the most sense and that everyone already follows. And I think I have it. There's the default parent targeting mode when nothing is set. And then there's either one of affect children only, target part or multi target part which will override the parent targeting mode, but otherwise they won't interfere with each other.

I hate to do things this way but in my opinion, even if I fold, that doesn't make any of you right. You wallow in poor design because we let it fester. Be thankful that I listen and find alternatives.

Apparently it's too much for people to uncheck their affect children only boxes.

TL;DR normal parent mode is default, but will be as usual bypassed out by every other mode. But the other modes will be independent of each other. Opine. I will wait for feedback a bit before making the changes.

pingu7867 avatar Oct 12 '24 06:10 pingu7867

I do not like your tone, so knock it off. You wouldn't be able to fix these issues without someone bringing it up in the first place.

I can't believe you in good faith. The event you highlight (the timerx) in your example doesn't even have children, there's only a target part. You expect me to believe it was a bad decision to make affect children only do what it says?

Pay attention to the image. I highlighted the timerx event just to show the setup. However, above the timerx event setup is the button events under "Gun Lock" one, that uses the exact same method. It is already known timerx do not function without ticking "affect children only", but this unintended feature in your eyes is the reason why we can organize events with target part like above. We know that even if they have ACO ticked, they will not interact with other events, but still affect targeted ones. Such function is more valuable than what you propose.

That exception is the reason this setup works overall. You can nestle your events to not interact with each other, yet still affect a target part just fine. It may not work exactly like proxies, but that is why some use events over them.

That's why target part now properly ignores its parent so you don't need to resort to affect children only to make it obey your downward structure. But at some point I have no qualms about forcing people to do things the way it's intended to.

I do agree that PAC should follow some rules, in order for it to work proper. However, I am very much in disagreement with your methods if you cannot accept criticism and concerns brought up.

The warnings will stay there out of spite to remind you not to use ACO if there are no children.

Cut the insult.

I hate to do things this way but in my opinion, even if I fold, that doesn't make any of you right. You wallow in poor design because we let it fester. Be thankful that I listen and find alternatives.

Do not start with me that attitude, you have no right to speak like this when someone voices valid concerns. Be thankful you get these concerns adressed to you in the first place, because not all people will sit idle and do nothing. You are an insult and you look at my concern as an inconvenience, just because someone had a problem with how you coded a worthless check that only breaks things instead of fixing it. You could have asked if you want to try the outfit for better insight, to see my point, but you reject that entirely and instead resort to insults. Aren't we supposed to discuss the concern at hand?

Undo the target part checks you added and let events remain they way they worked before. This change only brought more bad than good. And learn to accept you are not always in the right.

TheReeb avatar Oct 12 '24 11:10 TheReeb

ok I went a bit too far being belligerent defending my position. It's not personal though. I'm just saying just how much I think it doesn't make sense to use affect children only when there's no children. How I really feel is that, when people do hacky workarounds to make stuff work, it's more a symptom of a problem than it is a fault with them. Rather than arguing with everyone, it's better to fix the situation.

I'm not always in the right, it doesn't work like that. Being right about something and wrong about something else is quite possible. My addition of target part should've been properly implemented to bypass the default parent wiring to begin with. To do exactly the things you're talking about, which is both structuring and having timerx work properly. As you see, when you inspect closer for the root cause of the situation, it was my mistake. I may have fixed it afterward but the repercussions make it related to this issue.

More important than words is action. You're not the first one to find issue with this change either; and not the last, if I don't fix this. Which is why I said exactly how I'd take responsibility: reverting and finding a better solution. If I didn't hear people's feedback and take prompt action, I'd be worthless.

I just wanted to know before I push the patch, whether you think making the three modes (affect children, target part, multi target part) independent from each other so that they don't interfere with each other, is the perfect solution we need.

pingu7867 avatar Oct 12 '24 17:10 pingu7867

Alright, I do appreciate that you see what went wrong, and willing to cooperate. Let's not dwell on it, you're forgiven.

Now for the thing at hand: I do understand this was not an intended feature. However, if this has been around for so long that people see it as normal, and allows the user to get a little more creative with structuring events, I do believe it should remain a feature. Mistakes happen, but not all of them are.

The reason this work so well is it allows the user to construct multiple events next to each other, using ACO. Target Part allows the event to no longer be parented directly under what its looking for, and in a way, it reduces file size by shortening the parts tree length. It allows the user to organize their events in a very clean and efficient manner. I may have not clarified this clear enough, thus I apologize for the oversight.

Now I did see you have added a multiple target parts menu, but so far I am unable to get this working (I assume its not yet in that stage). If you intend to change so we can assign multiple targets, bound to a single event, then by all means I am welcoming that addition wholeheartedly.

Still, I believe keeping the unintended behavior might be best, and introduce the multi-target part functionality alongside. That way no existing PACs will be broken who happen to use this system, and a new method is introduced to the player to experiment with.

TheReeb avatar Oct 12 '24 18:10 TheReeb

But that's exactly what I'm trying to tell you. As soon as any target part is being used, parent routing should not apply. That specific aspect was fixed in this commit: https://github.com/CapsAdmin/pac3/commit/9a0a7167277a661bdb87dfef7a93f4d2fb8ed357

So you can structure how you want, just how it was meant to be. There'd be no more unintended behaviors. Maybe I'm playing semantics by redefining what's intended and unintended, but I should be transparent about my design process and about working through the principles that should decide the workflows.

To that end, firstly, I think I can observe that nearly all users understand and agree that the default parent targeting mode, which causes interference to events of the same parent (brothers/sisters if you will), should be bypassed any time an alternative wiring mode is used. Because these are presented as alternatives to the default wiring. I think that's tacitly known to just about everyone using any of these wiring modes. So I have one principle to start with.

Secondly, I've noticed how much people dislike when any option they use stops working. They don't really care or want to know if there are interactions between certain options. In other words, if one should do this and the other should do that, then adding this and that, there are two things happening. So, I notice a desire for a non-competing principle. The previous principle doesn't contradict this one, because in all cases, people see them as alternatives to the parent wiring mode, but they don't have reasons to believe they should interfere with one another.

Thirdly, from my perspective it was never intended that target part would still let the event affect its own parent. I just forgot to code that skip.

If you remember, I told you about why I thought the word "only" in "affect children only" should matter because linguistically it's a strong word and it should mean something. But experience will now dictate that no amount of principles or possible fixes will defeat an annoyed user base; they won't buy the silly beliefs. All that matters is: their habits and stuff have been disturbed, and they absolutely hate when they need to do more work. Now that's a strong principle.

So you see, there's no need to have unintended side effects. I only have to find the straightforward principles that most people operate under, and obey that to come back to what should've always been proper and intended. The reason why I think making all three alternate wiring modes independent will make everyone happy is that since they won't interfere with each other, it won't matter which combination you use. TP, that's good. TP + ACO, that's good, ACO + MTP that's good, etc. Neither would interfere with the parent, no matter what.

But like, if people built a habit of using ACO on timerx events, that doesn't get overturned if I make the alternate wiring modes non-competing. With or without ACO, using a target part will work, and it won't touch the parent.

FYI multiple target part already works, it uses a list of UIDs or names. It wasn't communicated as a tooltip or in the F1 entry before, but you can use bulk select (ctrl+click) to select some parts and right click on the part's tree label or on the text field to assign the list based on bulk selected items. I shall add more tooltip descriptions to clarify that.

pingu7867 avatar Oct 12 '24 19:10 pingu7867

But that's exactly what I'm trying to tell you. As soon as any target part is being used, parent routing should not apply. That specific aspect was fixed in this commit: https://github.com/CapsAdmin/pac3/commit/9a0a7167277a661bdb87dfef7a93f4d2fb8ed357

My bad, was a little confused but it's all clear now.

I am for the changes you've proposed, these are good ones. Looking forward to test changes.

TheReeb avatar Oct 12 '24 19:10 TheReeb

After some thorough testing of the changes implemented, everything work as intended. I don't really have much else to say, other than great work and thank you for the update.

TheReeb avatar Oct 15 '24 12:10 TheReeb