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

Return objects from events based extensions #3163

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

arthuro555
Copy link
Contributor

This adds an action to return objects from an events based extension. This will set the return value to true and pick the currently picked instances of the object back to where the function is used.

Example:
pick.zip

@arthuro555 arthuro555 requested a review from 4ian as a code owner October 10, 2021 10:03
@arthuro555 arthuro555 changed the title Return objects from events absed extensions Return objects from events based extensions Oct 10, 2021
@4ian
Copy link
Owner

4ian commented Oct 12, 2021

Finally getting to this! Very interesting! Is this supporting a few interesting use cases like objects created in the function, can you return them?

@arthuro555
Copy link
Contributor Author

Yep! You can test it with this project file: pikk.zip

@4ian 4ian marked this pull request as draft October 12, 2021 21:26
@4ian
Copy link
Owner

4ian commented Oct 12, 2021

I've identified multiple edge cases. This is not an easy problem, this can be very powerful but we must be sure that we handle everything properly and be 100% correct so that we don't discover later issues when using groups/passing groups/passing multiple objects/calling this action multiple times :)

I think the key is being very clear about what is inside the object lists.

@arthuro555
Copy link
Contributor Author

Whoops! While I indeed tested passing groups, I did not test for those edge cases. I fixed the first and second one, though I am not sure I understand the last one. If the object is passed twice, shouldn't modifying its picked objects still be fine? The only other alternative would be to not pick them but that sounds more confusing.

@arthuro555 arthuro555 requested a review from 4ian October 14, 2021 10:22
@arthuro555
Copy link
Contributor Author

I think I understand now what you meant with that last edge case (what if i am using a group in the function that consists of two times the same object that was passed?), and my current implementation while being a little slower on that edge case should be resilient against this (e.g. it will not duplicate objects in the list, since even if it fills it a second time it will clear it before doing so).

As the edge cases seem fixed, I am removing the draft.

@arthuro555 arthuro555 marked this pull request as ready for review October 23, 2021 21:28
@arthuro555
Copy link
Contributor Author

Ping!

1 similar comment
@arthuro555
Copy link
Contributor Author

Ping!

.AddAction("SetReturnObject",
_("Set object return value"),
_("Returns the currently selected objects. Those objects will be the ones selected for the next actions, conditions, subevents etc."),
_("Set objects to return to _PARAM0_"),
Copy link
Owner

Choose a reason for hiding this comment

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

We should probably find a name that implies that we're not really "returning" anything but rather "pick objects PARAM0 as the result of this condition"?
(speaking of which, this should usually only happen in a condition, even if in theory actions can also use this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it return because I think that is both more useful and intuitive. This action also sets the return value to true, so it does return "something". The logic behind it is that if you have no objects to pick, then a previous condition had 0 objects so it should be false, so you are not returning any objects and want your condition not to be true either. If you did pick objects in your previous conditions, then you change the picked objects and want the condition to return true since some objects are picked.

@4ian
Copy link
Owner

4ian commented Nov 25, 2021

Sorry about the delay on this, it's hard to review because I'm not sure if we're missing a case or not.
Can we somehow create a few exhaustive "test cases"? I'm thinking either of testing in GDJSCodeGenerationIntegrationTests or "just" a game that would use this in multiple settings:

  • With a group or not at the scene level
  • With groups or not inside the function
  • Passing groups with multiple times the same object
  • Maybe a final test case where a function calls another functions, and both use the action (to ensure picked objects can traverse 2 layers of functions? And that there is no problem of naming, especially because we're pushing objects using their names in the object lists).

@D8H
Copy link
Collaborator

D8H commented Jul 23, 2022

I added some cases in the example project:
https://www.dropbox.com/s/4gk4gpzcho2ohve/PickTest.zip?dl=0

I observed that:

  • It doesn't feel intuitive with behavior functions
    When this action is used in a behavior function, it will be called for each owner instance. The last instance executed will decide what is picked.
    Most of the time, extension users will surround the behavior action with a for each to handle the picked instances with knowledge of the behavior owner. But, in case they don't need to, the function will seem broken.

  • An event-based condition A that call another event-based B won't work because the condition B doesn't find the object lists.

  • Maybe not related to this PR: when an object is created in the picked instances are reset
    One use-case is the bullet extension. It kind of warp a create action and I guess users will expect it to work the same. This means adding the picked object to the picking list and keeping objects that were already picked.

@D8H
Copy link
Collaborator

D8H commented Oct 22, 2023

To answer my younger self:

  • It doesn't feel intuitive with behavior functions
    When this action is used in a behavior function, it will be called for each owner instance. The last instance executed will decide what is picked.
    Most of the time, extension users will surround the behavior action with a for each to handle the picked instances with knowledge of the behavior owner. But, in case they don't need to, the function will seem broken.

This is actually not a real use-case because behavior Object picking is handled by the generated code according to returned boolean from the condition already.
The UI should just hide the "Return object" action from within behaviors.

  • An event-based condition A that call another event-based B won't work because the condition B doesn't find the object lists.

The issue was the use of object.getName() which gives the object name in the scene but it didn't work if the event-function caller is not a scene event.
I tried a different implementation to fix this issue:

It will work for simple picking but the events may need to use imbricated "for each instance" to check some conditions on each object tuples. It won't be possible with just this new action.

Maybe, it could require a new concept like named picking list:

  • An action Add Object to the object list "MyPickingList"
  • An action Remove Object from the object list "MyPickingList"
  • A condition Pick all Object from the object list "MyPickingList"

The idea would be to build an object list in "for each instance" loops and use the condition to get the result and give it to the "Return" action. Something like this:

Clear the object list "MyPickingList"
for each instance of Player
  for each instance of Obstacle
    A condition on Player and Obstacle --> Add Object to the object list "MyPickingList"
                                       --> Add Obstacle to the object list "MyPickingList"
Pick all Player from the object list "MyPickingList" --> Set objects to return to Player
Pick all Obstacle from the object list "MyPickingList" --> Set objects to return to Obstacle

Though, it would be better if we can found a simpler solution.

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

3 participants