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.

Sign up for GitHub

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

Jump to bottom

Allow to use parameters and properties in the variable action and condition #7124

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

Allow to use parameters and properties in the variable action and condition #7124

wants to merge 21 commits into from

Conversation

D8H
Copy link
Collaborator

@D8H D8H commented Nov 3, 2024 •
edited
Loading

Changes

  • Properties and parameters can be used in the primitive variable conditions
  • Properties can be used in the primitive variable action
  • Change the priority to:
    • Global variables
    • Scene variables
    • Properties
    • Parameters
    • Local variables

Technical solution

  • Properties and parameters are used to generate the VariablesContainersList.
    • It allows to finely choose the priority.
    • It's consistent to what is already done for the ObjectsContainerList.
  • Variable instructions uses 2 new parameter types variableOrPropertyOrParameter and variableOrProperty.
    • Instructions that use variables are references still use the parameter type variable.

Manual tests

  • Ran templates without any change in the events:
    • 3D car coin hunt
    • 3D lane runner
    • 3D shark frenzy
    • Papa is you
    • 360° platformer
  • Ran templates with all extensions migrated to variable instructions:
    • 360° platformer
      image

@D8H D8H force-pushed the property-as-variable branch from f676bd5 to 9a7b060 Compare November 16, 2024 21:06
@D8H D8H force-pushed the property-as-variable branch 2 times, most recently from ec408b7 to 6c098a5 Compare November 25, 2024 17:58
@D8H D8H force-pushed the property-as-variable branch 2 times, most recently from de90712 to 966c9d2 Compare December 13, 2024 15:16
@D8H D8H marked this pull request as ready for review December 13, 2024 16:25
@D8H D8H requested a review from 4ian as a code owner December 13, 2024 16:25
@D8H D8H force-pushed the property-as-variable branch 3 times, most recently from de21147 to be8c37f Compare December 13, 2024 18:58
4ian
4ian reviewed Dec 15, 2024
View reviewed changes
Core/GDCore/IDE/Events/ExpressionValidator.cpp
@@ -460,6 +460,8 @@ const gd::String& ExpressionValidator::TypeToString(Type type) {
case Type::NumberOrString:
return numberOrStringTypeString;
case Type::Variable:
case Type::VariableOrProperty:
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a risk that the caller of this function would expect to get back something like "variableOrProperty" or "variableOrPropertyOrParameter"?

4ian
4ian reviewed Dec 15, 2024
View reviewed changes
Core/tests/WholeProjectRefactorer.cpp
@@ -136,6 +136,74 @@ CreateInstructionWithVariableParameter(gd::Project &project,
return event.GetActions().Insert(instruction);
}

const gd::Instruction &
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the exhaustive tests 👍

@D8H D8H force-pushed the property-as-variable branch from ef546d2 to 3fa471c Compare December 18, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@4ian 4ian 4ian left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@D8H @4ian