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

Add trigger once per instance #5435

Closed
wants to merge 5 commits into from

Conversation

arthuro555
Copy link
Contributor

Adds a trigger once per instance of an object condition.

It's a very important building block and allows for all kinds of logic where the normal trigger once has been oftentimes misused by the community causing bugs and issues in their games.

@arthuro555 arthuro555 requested a review from 4ian as a code owner June 21, 2023 15:53
Copy link
Owner

@4ian 4ian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm overall. @D8H can you take a look?

@D8H
Copy link
Collaborator

D8H commented Aug 14, 2023

I did some researches a while ago to try to understand the need. On the forum, almost every question about "trigger once" inside "for each object" loops I could find could not have been solved with a "trigger once per instance". It was often a beginner not understanding 1 of the 2 concepts and the answers was to remove 1 of them.

There was still an interesting use-case: in a tower defender, each Spikes object deals one damage to each Ennemy traveling on them. I've made a quick example so we can experiment a bit because "trigger once" are, in my opinion, very abstract and hard to predict.

With this examples, my take on "trigger once" (common one and per instance one) is:

  • They are convenient (like any anti-pattern is)
  • Users would be better off using variables to track their states. Variables are more verbose but they are palpable, you know what you're doing.
  • I guess most basic cases can be solved by "has just been" conditions. And I think we should make sure that users have every condition they need not to use "trigger once" and deprecate the "trigger once" after this (I know this is a bit extreme but I really think it will simplify users life).

I'm very interested in discussing other usecases because I avoid using "trigger once" as much as possible as I don't understand how they work and I don't have much experience with them as a result.

@arthuro555
Copy link
Contributor Author

First thing that comes to mind: State machines
image

obviously, for this usage a better way would be to be able to use callback functions to call when the state has just been changed because events like these could lead to a state having their initialization functions called only at the next frame or generally have these called out of order making for annoying hard to debug issues, but we do not have callback functions.

I am puzzled as to how you do not see a use to them, they are generally very useful most of the time? This function is one I've wanted for years personally and I've had to write almost equivalent code as if there was a trigger once per object, except much more unreadable, fragile and taking up more event space.

I am equally puzzled at your suggestion - we should make duplicated variants of conditions that trigger once, instead of having a composable & optional separate trigger once condition?
...Really? IMHO, this is exactly what we should avoid. One of the biggest productivity killers and annoying mistakes to make in GDevelop is having 3 duplicates of every variable condition for every scope. I have unironically wasted hours with mistakes of a single action/condition being for the wrong scope, or looking through the list of variables conditions after searching for "variable" and trying to take the one for the right scope. This is the one biggest pain point for me in GDevelop. It would be much better as far as I am concerned to have a way to just specify the scope within the actions/conditions parameters.
Making a lot of duplicate actions is a burden for contributors and maintainers, unnecessarily bloats the interface, makes GDevelop more of a pain to use... It's just a pain for everyone?

I don't really get how this is an anti-pattern either? It's a convenience feature for a common logic use case, wanting to do something only once, e.g.
image

@D8H
Copy link
Collaborator

D8H commented Aug 14, 2023

I don't really get how this is an anti-pattern either?

Did you try the pause hack in the example I linked?
It allows players to deal damages by spamming the pause button.
I think this is a very common mistake done by users. They think that the "trigger once" only applies to the condition from the same event when actually every parent condition matter. This means that a state change of a FSM is resetting any trigger once which can be an issue.

First thing that comes to mind: State machines

I think the best way to make a FSM on objects is to make a custom behaviors. It doesn't need "trigger once" because the transition function knows the initial state (the property value) and the new state (the parameter or the function).

I am puzzled as to how you do not see a use to them

In my experience, they are easy to use on simple project when they are almost used at root events, but as soon as there is more depth, it's hard to predict their side effects. The worst is probably when "trigger once" are nested.

@arthuro555
Copy link
Contributor Author

I see, I understand the problem. I still do not think your solution is appropriate. Here is my take on solving this problem:

image

This is explicit about what is to be included by the trigger once. The condition would trigger if the conditions it contains are true AND the conditions were not true the last time the condition was reached.

While it'd be fine to push that explicit trigger once over the current ones in beginner tutorials, would still not deprecate the trigger once or block the trigger once per instance - for an informed user that understands completely how the event sheets works, these can be a very useful tool, for example this explicit trigger once wouldn't allow you to do something like

image

@HelperWesley
Copy link

Ok, my 2 cents. I don't know what to think of "trigger once per instance", but I'm gonna defend the original "trigger once".

In a real game, you wouldn't use trigger once for taking damage, you'd use iframes(Usually paired with an animation) to prevent the player from taking repeated damage for a duration of time. So pausing and unpausing would not be the condition for this, if someone used it that way then that's no different than if they used a variable to "act" as a trigger once and did the same thing.

Using variables in place of trigger once would be an absolute nightmare of stray variables with random names that people make up on the fly to use in place of trigger once. And then to repeat the action you'd need to reset the variable. 😕

@D8H
Copy link
Collaborator

D8H commented Aug 15, 2023

@arthuro555

I see, I understand the problem. I still do not think your solution is appropriate. Here is my take on solving this problem:

Do you think this nuance also applies to the "per instance" one?
If so do you think users could be confused about it? Do you have a any preference between the 2?

@HelperWesley

In a real game, you wouldn't use trigger once for taking damage, you'd use iframes(Usually paired with an animation) to prevent the player from taking repeated damage for a duration of time. So pausing and unpausing would not be the condition for this, if someone used it that way then that's no different than if they used a variable to "act" as a trigger once and did the same thing.

Using a condition that is the negation of the action result is a good way to avoid using "trigger once", I do this too. My bet is that most of the time, especially when objects are involved, "trigger once" can be avoided this way and worst case scenario a variable must be used but I think the variable can be useful at other places and maybe make the code easier to follow and more predictable. That's said, it's just a gut feeling and I have no usecases to support it.

Using variables in place of trigger once would be an absolute nightmare of stray variables with random names that people make up on the fly to use in place of trigger once. And then to repeat the action you'd need to reset the variable.

I should take a look to the example base, but I guess most "trigger once" usages are on inputs handling which could be solved by a "Key just pressed" condition. But, here again, it's just a feeling.

@arthuro555
Copy link
Contributor Author

I propose that we carry this discussion on in a github discussion. We can discuss if we want to remove trigger onces there, but seeing as it is mostly a sentiment of yours and that in the meantime trigger once remains for now a primary tool of GDevelop, I do not believe it is pertinent to block this PR over this. This PR, in the meantime, already concretely solves problems users often have, and is a step forwards in terms of improving the trigger once situation. It'd wouldn't change much to deprecate one more condition if that is the path that we'll go down into in the future, but it'd help users a lot now to have this feature.

Instead of banging their head about why this:
image
is not working as they would assume, they'd have an easy to use condition that does exactly the thing they assume the events above would do, improving user experience.

@tristanbob
Copy link
Contributor

Here is a use case:
How can I make sure that each time a bullet hits an enemy, exactly 1 HP is lost.
This should work even when multiple bullets are hitting multiple enemies at the same time.

chrome_n8cuC18ivD.mp4

@tristanbob
Copy link
Contributor

tristanbob commented Aug 15, 2023

From a user visibility standpoint, I don't think we should create a second condition with a very similar name. This will confuse a lot of users.

Instead, can we keep a single "Trigger once" condition with conditional behavior?

  • Outside ForEach loop: No change
  • Inside a ForEach loop: New "per instance" behavior is used

I think this is more intuitive to the users.

@arthuro555
Copy link
Contributor Author

We cannot do such a breaking change to such a core part of the engine. This would break thousands of project at least.

@tristanbob
Copy link
Contributor

We cannot do such a breaking change to such a core part of the engine. This would break thousands of project at least.

We can hide the old version of the condition so that there are no changes to existing games.
The new TriggerOnce would only be used when someone adds it to an event.

@D8H
Copy link
Collaborator

D8H commented Aug 15, 2023

How does the condition of this PR works when several are used together? Does the order matter?

@arthuro555
Copy link
Contributor Author

We can hide the old version of the condition so that there are no changes to existing games.
The new TriggerOnce would only be used when someone adds it to an event.

That would be terribly confusing to all current users of GDevelop to see the behavior of that tool they have grown to learn change suddenly, and have different trigger onces in their projects act differently.

It would also break an unsaid implicit rule of GDevelop - regardless of where it is placed, any event, action or function never modify their own behavior. Higher up events can change how they are called - with what objects lists, how many times, if they are called at all... But the events down the line themselves never change as a part of that. This would make the trigger once, I believe, confusing, as it would make it harder to your head around it since it would deviate from the behavior of literally all other events in GDevelop.

How does the condition of this PR works when several are used together? Does the order matter?

Several what? Used how? The order of what?

@D8H
Copy link
Collaborator

D8H commented Aug 15, 2023

How does the condition of this PR works when several are used together? Does the order matter?

Several what? Used how? The order of what?

When users want to trigger once per couple of 2 objects. I guess they will put 2 "trigger once per instance" next to each other.
Does it work as users may expect?
Does the order of these 2 "trigger once per instance" matter?

@arthuro555
Copy link
Contributor Author

Does it work as users may expect?

This condition is not made to be used on couples of object and would not work as expected on those, no. Generally though, GDevelop is very bad at handling couples of objects and we'd have no way to tell from the objects lists alone which instance of what objects started to match conditions with each other.

If object couples. e.g. a collision condition, used somehow a list of couples internally instead of generally the instances that are coupled to one or many of the objects in the other instances list, it would become possible to handle as expected, and it would also probably allow to fix issues like:

  • Being able to target the instances that are colliding with every specific instance without having to use a for each which may hinder the performance of the collision condition with future implementations
  • Being able to do collisions between instances of the same object type

Does order matter?

It shouldn't, since both are operating on separate objects lists and just filtering out the instances that had reached that condition already last frame.

@D8H
Copy link
Collaborator

D8H commented Aug 16, 2023

I'm afraid that users ask for "trigger once per instance" because they are used to "trigger once" but if they had one, they would realize that it's actually too limited for what they want to do.
I would like that we find some usecases where the "trigger once per instance" is useful to make this fear disappear.
I worked on many examples and never felt the need for a "trigger once per instance", this is why I'm asking this.

First thing that comes to mind: State machines image

obviously, for this usage a better way would be to be able to use callback functions to call when the state has just been changed because events like these could lead to a state having their initialization functions called only at the next frame or generally have these called out of order making for annoying hard to debug issues, but we do not have callback functions.

So, "trigger once per instance" is not really a good solution for FSM. Without using a custom behaviors, I guess a good way is to:

  • have 2 variables: State and PreviousState
  • check all transitions at the beginning using PreviousState in conditions and State in actions
  • compare State and PreviousState instead of a "trigger once"
  • apply state effects

I don't really get how this is an anti-pattern either? It's a convenience feature for a common logic use case, wanting to do something only once, e.g. image

Here, a trigger once is not needed. It can be replaced by "YouWon is not visible" which avoid any side effect of a parent condition.

@Silver-Streak
Copy link
Collaborator

Silver-Streak commented Aug 17, 2023

TLDR:

Just because trigger once (per instance or the existing general one) could be accomplished by variables does not mean that variables are an easier or better workflow for triggering an event only once until the conditions are false. There are no "per event" variables, so trying to replace trigger once with variables could lead to dozens (if not hundreds) of extra variables in medium sized games. It would be a workflow nightmare, not to mention a QOL one.

More detail:

Trigger once per instance is an extremely common ask on the forums and discord which seems like there would be numerous uses, I am unclear why there is hesitation?

Examples questions I've seen over the last few weeks:

  • User wants to add 5 to a red tint color to an instance of object A the first frame colliding with an instance of object B, but not again until they are no longer in collision. There are multiple instances of A and B.
  • User is making a bullet hell game and wants to make each bullet do damage to the player once it hits them (one frame). But keep them valid to hit again once they are no longer in contact (as they are spinning around the screen). All bullets types are in a single bullet group.

Yes, all of these things could be done with boolean variables for tracking. That is also true for the normal trigger once condition. The value is in simpification of workflow and standardization of methods between handling one instance logic and multi instance behavior logic, or when object lists built off a singular condition don't apply.

Edit: Also, the trigger once condition was added in GD4 explicitly to avoid having to track variables for this kind of stuff. https://forum.gdevelop.io/t/trigger-once-condition-block/9256/5

@D8H
Copy link
Collaborator

D8H commented Aug 19, 2023

Trigger once per instance is an extremely common ask on the forums and discord which seems like there would be numerous uses, I am unclear why there is hesitation?

User requests give a problem not a solution, especially for something as abstract as "trigger once".
I want to make sure there are actual use-cases where a "trigger once per instance" will be useful.

  • User wants to add 5 to a red tint color to an instance of object A the first frame colliding with an instance of object B, but not again until they are no longer in collision. There are multiple instances of A and B.

"trigger once per instance" doesn't work to trigger once per couple of instances. I think it will be a common mistake about it.

  • User is making a bullet hell game and wants to make each bullet do damage to the player once it hits them (one frame). But keep them valid to hit again once they are no longer in contact (as they are spinning around the screen). All bullets types are in a single bullet group.

A bullet that is not destroyed on hit is peculiar but why not.
Do you have other useceses in mind?

@Silver-Streak
Copy link
Collaborator

Silver-Streak commented Aug 19, 2023

A bullet that is not destroyed on hit is peculiar but why not.

To clarify, it is pretty common in top down bullet hell shooters, especially those in the "Touhou" series of games, as well as "bullet heaven" style games like Vampire Survivor (Spinning books weapon in Vampire Survivor works this way)

@D8H
Copy link
Collaborator

D8H commented Aug 25, 2023

A bullet that is not destroyed on hit is peculiar but why not.

To clarify, it is pretty common in top down bullet hell shooters, especially those in the "Touhou" series of games, as well as "bullet heaven" style games like Vampire Survivor (Spinning books weapon in Vampire Survivor works this way)

It won't work for books as there are multiple Book and Enemy. This is why I'm not found of this solution. I'm sure it can simplify some cases but they seem rare and very similar cases won't work. It will be hard to explain to users when they can use it. The existing trigger once is confusing a lot of users. The trigger once per instance will likely confuse them too.

Should users keep track of what hit what with the LinkedObject extension? It's more verbose but at least it works for couple of objects too. Links are probably easier to debug. Users can access the links and draw a line between linked objects for instance (it could be an extension).
LinkedObject instructions are a bit buried but we could show them in the object instruction list to make them easier to find.

@Silver-Streak
Copy link
Collaborator

It won't work for books as there are multiple Book and Enemy. This is why I'm not found of this solution. I'm sure it can simplify some cases but they seem rare and very similar cases won't work. It will be hard to explain to users when they can use it. The existing trigger once is confusing a lot of users. The trigger once per instance will likely confuse them too.

Unless I'm misreading the change here, wouldn't this allow you to do:
For each Monster >
pick all books + Books in collision with monster + trigger once for each instance of book > Apply damage.

@D8H
Copy link
Collaborator

D8H commented Aug 25, 2023

It won't work for books as there are multiple Book and Enemy. This is why I'm not found of this solution. I'm sure it can simplify some cases but they seem rare and very similar cases won't work. It will be hard to explain to users when they can use it. The existing trigger once is confusing a lot of users. The trigger once per instance will likely confuse them too.

Unless I'm misreading the change here, wouldn't this allow you to do: For each Monster > pick all books + Books in collision with monster + trigger once for each instance of book > Apply damage.

The same way "trigger once" can't be used inside a for each, "trigger once per instance of ObjectB" can't be used in "for each of ObjectA".
This is because "trigger once per instance" can only store a boolean state for each instance when there would need to store a boolean for each couple (ObjectA, ObjectB).

To be generalized to any number of objects, the trigger once per instance would need a way to know which instances are involved. It could be done by looking for parent "for each" like Tristan suggested, but it probably has its challenges too.

It's really not trivial and must be thought carefully.

@arthuro555
Copy link
Contributor Author

arthuro555 commented Sep 3, 2023

I'll reiterate: whether trigger once is good or not is irrelevant right now. Fact of the matter is, trigger once is in GDevelop right now and wisely used, and we cannot realistically remove it or deprecate it in the near future. In this context, this PR has a positive value: it allows some events that were previously broken to be corrected.

It doesn't matter if it doesn't fix cases with two or more objects, since that would need to be a separate condition either way and it is impossible to implement for now anyways. Heck, even if using two trigger onces after each other defies GDevelop common sense and obviously wouldn't work with the rules of the event sheets, we can add a warning that "Placing two trigger once per instance will not trigger once per pair of object" if it bothers you that much.

If we all someday converge on trigger once being bad and a strategy to remove/replace it, having one extra trigger once condition to deprecate will change nothing - it can be treated exactly the same as the classic trigger once. In the meantime, this allows for more correct use of the trigger once, refusing to merge this over not liking the trigger once doesn't make sense.

So, again, let's move all this discussion, whether or not we want to remove trigger once, the strategy for removing it without breaking everyone's stuff, finding all the ways trigger once is used and defining the "correct way to do it instead" for each of them, etc. over to a github discussion. In the meantime, this PR will help improve the state of trigger once even if it does not remove the "root of the evil".

@arthuro555
Copy link
Contributor Author

Welp this is getting nowhere

@arthuro555 arthuro555 closed this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants