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

[Auto PR] New extension: Only Once Condition For Object #281

Closed
wants to merge 9 commits into from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 5, 2021

Description

This extension is useful if you want to trigger only once per instance.
This is easier than creating your own instance variables.
In addition, it does not require any action to set it to True.

How to use the extension

Adding the "Only Once Condition" behavior to an object will add the "Only Once" condition to the instance. The condition will be identified by the condition name and will be executed only once.

Unlike the "Trigger once while true" condition, this condition stays true until reset.

condition

Checklist

  • I've followed all of the best practices.
  • I confirm that this extension can be integrated to this GitHub repository, distributed and MIT licensed.

Example

OnlyOnceConditionForObject.zip

Extension file

OnlyOnceConditionForObject1.0.0.zip

@github-actions github-actions bot added the ✨ New extension A new extension label Nov 5, 2021
@github-actions github-actions bot added this to Needs review in Extensions review Nov 5, 2021
@PANDAKO-GitHub
Copy link
Contributor

Fixed some descriptions.
v1.0.1

OnlyOnceConditionForObject1.0.1.zip

@arthuro555
Copy link
Member

!update OnlyOnceConditionForObject1.0.1.zip

@github-actions
Copy link
Contributor Author

github-actions bot commented Nov 6, 2021

A build error occured while processing the extension update:

❌ 1 Error found in extension 'OnlyOnceConditionForObject':

  ⟶ ❌ [Extension name consistency]: Extension filename should be exactly the name of the extension (with .json extension). Please rename 'OnlyOnceConditionForObject1.0.0.json' to 'OnlyOnceConditionForObject.json'.

❌ 1 Error found in extension 'OnlyOnceConditionForObject':

  ⟶ ❌ [Extension name consistency]: Extension filename should be exactly the name of the extension (with .json extension). Please rename 'OnlyOnceConditionForObject1.0.1.json' to 'OnlyOnceConditionForObject.json'.


❌ 2 Errors found in extensions - please fix them before generating the registry.

@PANDAKO-GitHub
Copy link
Contributor

I don't know what's happening here. 🤔
Should I follow the bot's instructions?

I renamed the file as instructed by the bot.
v1.0.1
OnlyOnceConditionForObject.zip

@arthuro555
Copy link
Member

Don't worry about those, I can take care of them

@Bouh
Copy link
Contributor

Bouh commented Nov 29, 2021

I updated the extension file.
I don't get the point of the checkboxes moving in the example and how are they affected by the extension.

@Bouh
Copy link
Contributor

Bouh commented Nov 29, 2021

!update OnlyOnceConditionForObject.zip

@github-actions
Copy link
Contributor Author

✅ Successfully updated the extension.

@github-actions
Copy link
Contributor Author

❗ An internal error has occured. See logs at https://github.com/GDevelopApp/GDevelop-extensions/actions/runs/1515692205.

@Bouh
Copy link
Contributor

Bouh commented Nov 29, 2021

!rebase

@github-actions
Copy link
Contributor Author

✅ Successfully rebased from main.

@Bouh
Copy link
Contributor

Bouh commented Nov 29, 2021

!update OnlyOnceConditionForObject.zip

@github-actions
Copy link
Contributor Author

✅ Successfully updated the extension.

@Bouh
Copy link
Contributor

Bouh commented Nov 29, 2021

!update OnlyOnceConditionForObject.zip

@github-actions
Copy link
Contributor Author

✅ Successfully updated the extension.

@Bouh Bouh moved this from Needs review to Approved - awaiting merge in Extensions review Nov 29, 2021
@Silver-Streak
Copy link
Contributor

I'd recommend instead of "Only once" it be named either "Once per instance", "Once per object instance", or "Trigger once per object instance". Without explicitly calling out the "Per Instance" or "Per Object Instance", users may confuse its use case if using the right-click version of the condition/action editor.

"Trigger once per object instance" might be the most uniform one that helps people connect it with the existing Trigger Once. (Similar to how a "For Each Object" event is still listed alongside a normal "Event")

Description wise, I'd probably recommend updating it to:
"Only trigger the first time all conditions in this event are true for an object instance. Unlike the "Trigger once while true" condition, this condition stays true until reset."

@4ian 4ian self-requested a review November 29, 2021 21:47
Copy link
Collaborator

@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.

Agree on what @Silver-Streak said. This should be per instance. Also because there is an explicit name requested, I'm not sure it works like a trigger once. A trigger once will work per event and does not need to be reset. So here it should be made clear this should be reset.

Extensions review automation moved this from Approved - awaiting merge to Needs changes Nov 29, 2021
@PANDAKO-GitHub
Copy link
Contributor

Thank you all for your reviews and fixes.

I don't know if I'm allowed to fix this, but I did.

I've changed "Only once" to "Trigger once".
I have also changed the file name and extension name.

"Only trigger the first time all conditions in this event are true for an object instance."
This description is in danger of being slightly misunderstood.

When this condition is checked, it will return false the next time this condition is checked, even if the event was not executed due to other conditions.
(This process is the same as "Trigger once while true")
For example, this is the same reason why this event does not work.

2021-11-30 180007

TriggerOnceConditionForObject.zip

@arthuro555 arthuro555 moved this from Approved - awaiting merge to Needs review in Extensions review Jan 21, 2022
@Bouh Bouh self-requested a review January 21, 2022 13:00
"objectGroups": []
},
{
"description": "Reset the condition of the Condition Name parameter.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Reset the condition of the Condition Name parameter.",
"description": "Reset the specified "Trigger once per instance" condition by a the Condition Name parameter.",

Copy link
Contributor

Choose a reason for hiding this comment

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

In the action Reset condition the events are false in any case.
image

The events can be reduced to one line?

"objectType": "",
"eventsFunctions": [
{
"description": "The condition name of this instance was checked for the first time.\nUnlike the \"Trigger once while true\" condition, this condition stays false until reset.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "The condition name of this instance was checked for the first time.\nUnlike the \"Trigger once while true\" condition, this condition stays false until reset.",
"description": "The condition name of this instance was checked for the first time.\nUnlike the \"Trigger once while true\" condition, this condition stays false until reset manually with the appropriate action.",

It's automatic or not, an innocent user doesn't know it, it must be specified.

@Bouh Bouh requested a review from a team January 23, 2022 17:11
@4ian
Copy link
Collaborator

4ian commented Jan 23, 2022

I still feel like it's a bit risky because it works differently than the "classic" trigger once. Shouldn't we somehow look at implementing it the same way (this might require advanced stuff in JS)?

@PANDAKO-GitHub
Copy link
Contributor

@Bouh , thank you for your review.
I have revised the description and removed one unnecessary event.

!update TriggerOnceConditionForObject.zip

@4ian , thank you for your review.
I don't have a good idea.
I like the way it is now, where I can control when to reset it myself, just like a timer.
But this is just my view.

@github-actions
Copy link
Contributor Author

❗ An internal error has occured. See logs at https://github.com/GDevelopApp/GDevelop-extensions/actions/runs/1744651985.

@arthuro555
Copy link
Member

Whoops looks like my update to the commands broke the update command when used after a rebase command 🤦‍♂️

@4ian
Copy link
Collaborator

4ian commented Jan 25, 2022

@arthuro555 Any chance you can fix that? :)

@arthuro555
Copy link
Member

I'm... Not sure? It's about git refusing to revert a merge commit, and I'm not knowledgeable enough about git to understand what the -m option does exactly, which according to the output message is what's needed to make it work.

@4ian
Copy link
Collaborator

4ian commented Jan 26, 2022

Let's give this a new try:

!update TriggerOnceConditionForObject.zip

@github-actions
Copy link
Contributor Author

✅ Successfully updated the extension.

@4ian 4ian force-pushed the extension/PANDAKO-GitHub/280 branch from 934675f to 0edc57f Compare January 26, 2022 09:47
@GDevelopApp GDevelopApp deleted a comment from github-actions bot Jan 26, 2022
@GDevelopApp GDevelopApp deleted a comment from github-actions bot Jan 26, 2022
@GDevelopApp GDevelopApp deleted a comment from github-actions bot Jan 26, 2022
@4ian
Copy link
Collaborator

4ian commented Jan 26, 2022

!update TriggerOnceConditionForObject.zip

@github-actions
Copy link
Contributor Author

✅ Successfully updated the extension.

@GDevelopApp GDevelopApp deleted a comment from github-actions bot Jan 26, 2022
@4ian
Copy link
Collaborator

4ian commented Jan 26, 2022

@arthuro555: tl;dr, there is no way to automatically "rebase" after a name change after main was merged.
I renamed the "rebase" command to "merge" because it is what it's doing.

I was about to add a "hard-reset" command that would hard reset to main and add the changes (so any filename change is updated properly :)) but this means anyone can come and hard reset a branch with their changes and "lose" the history made by others. Not great => can we limit a command to maintainers? If some we could add this "hard-reset" command back and just use it when we identify its either the original author or us (maintainers).

@arthuro555
Copy link
Member

Else, we could remove the rebase command completely, removing any future issues, since it was mostly there to regenerate the header file before merging which has little point now.

@D8H
Copy link
Contributor

D8H commented Jan 26, 2022

I wonder if avoiding one action is worth introducing a new concept. I'm personally happy handling boolean variables. The "Condition Name" won't have autocompletion for instance. I personally think that there will be more time lost than saved.

What is your view on this?

@arthuro555
Copy link
Member

Hello @PANDAKO-GitHub 👋

A review has started for this extension, but a new option has presented itself in the meantime. We now have a list of unreviewed "Community extensions", and your extension can be added to it, bypassing the review phase.

If you chose to not take a review, you need to keep the following in mind:

  • As of now, community extensions are not yet shown to users, but they should be until next update
  • Community extensions will never be shown to GDevelop users directly, they will have to check a checkbox for them to be shown
  • All community extension have a warning about them being unstable and unreviewed displayed on their store page
  • Your extension will be added immediately, no question asked, and without you needing to do any changes to your extension

tl;dr You will not need to do any more work on your extension, but it might not meet the standards of all users, and reach less users.

Your case is a bit special, so taking the review will be different for you: since your extension provides such an important feature for GDevelop, if you chose to take the review, the extension might sadly never get added, as the reviewers would prefer a version of this condition that acts more like the normal trigger once, and that cannot be made with a normal extension. The option of taking the review for your would basically mean abandoning the extension and waiting for someone to add it to GDevelop itself, not as an extension.

The choice is yours, please tell us how you would like to continue.

@PANDAKO-GitHub
Copy link
Contributor

Hi @arthuro555 😊
I hope this is added to GDevelop itself, not as an extension.
Doing so will also solve the auto-complete problem.

@arthuro555
Copy link
Member

Ok! I'll be closing this submission then.

@arthuro555 arthuro555 closed this Jul 4, 2022
Extensions review automation moved this from Needs review to Rejected Jul 4, 2022
@tristanbob
Copy link
Contributor

This subject is also being discussed here:
4ian/GDevelop#3647

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ New extension A new extension
Projects
Extensions review
  
Rejected
Development

Successfully merging this pull request may close these issues.

None yet

7 participants