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

[Reviewed] [Shock wave effect] No longer require the Tween behavior #1003

Merged
merged 2 commits into from
May 4, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 6, 2023

Description

Will allow you to draw shock wave effects.

Changes

  • No longer require the Tween behavior.

Breaking change

  • Extension, behavior and function names have change.

How to use the extension

This behavior must be assigned to a shape painter object.
The user will have state parameters (Start and End) like the particle emitter object, which are :

  • Type (Fill or line)
  • The color transition (Start / End).
  • The opacity transition (Start / End).
  • The transition of angle (Start / End).
  • The transition of the radius of the explosion (Start / End).
  • The transition of the contour size (Start / End).
  • The duration of the propagation.

The user can modify the values according to his needs.

You can modify the value of the object just after the creation of the object in the event sheet (all modifications are applied on the shape painter object created above the modification).

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.
  • I am aware that the extension may be updated by anyone, and do not need my explicit consent to do so.

What tier of review do you aim for your extension?

Reviewed

Example file

DarwShockWaveEffect-Exemple.zip

Extension file

ShockWaveEffect.zip

@github-actions github-actions bot requested a review from a team as a code owner September 6, 2023 09:10
@github-actions github-actions bot added the ✨ New extension A new extension label Sep 6, 2023
@github-actions github-actions bot added this to Needs review in Extensions review Sep 6, 2023
@github-actions github-actions bot mentioned this pull request Sep 6, 2023
3 tasks
@D8H D8H added 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging. 🔄 Extension update An update for an existing extension and removed ✨ New extension A new extension 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging. labels Sep 6, 2023
@D8H
Copy link
Contributor

D8H commented Sep 7, 2023

Very cool extension, nice to see it updated!

Do you think it could be useful to choose an easing per value?

Some suggestions:

  • I think it's more user-friendly to have the start and end properties next to each other (in the same group).
  • Width/Height and Color/Opacity properties will be in 2 columns if they are in the same group.
  • Cheating the property ordering is not a good idea because it will make your life difficult and the issue will be solved one day.
  • If possible it's better to use hidden properties rather than variables (but it's not necessary to change it).
  • normalize can be replaced by a division.
  • The event with the "trigger once" can probably be removed without any effect.
  • There is no need to check the time as the object is destroyed automatically.
  • Time change should happen after the drawing or the initial state will never be shown.
  • doStepPostEvent should be used instead of doStepPreEvent to avoid a 1-frame delay (more generally for anything rendering related).
  • The draw ellipse action is the same in both cases.
  • Are the floor necessary?
  • Actions, conditions and expressions can be generated from the properties.
    image

I have the impression that a particle emitter with an image of a star would be easier to use than the star behavior. What is your view on the matter?

@Alios5
Copy link
Contributor

Alios5 commented Sep 8, 2023

OK I've started the modification I've almost finished the ellipse

@Alios5
Copy link
Contributor

Alios5 commented Sep 8, 2023

@D8H the floor is for pixel perfect animation

@tristanbob
Copy link
Contributor

@Alios5 thanks for improving your already awesome extension. What is the status of this update? I'd love to get it into GDevelop!

@D8H
Copy link
Contributor

D8H commented Nov 22, 2023

Please attach a new version of the example that uses the last extension update.

Edit: sorry, the bot did It. I'll check it soon.

@D8H
Copy link
Contributor

D8H commented Nov 22, 2023

Some of my previous suggestions are unanswered:

  • I think it's more user-friendly to have the start and end properties next to each other (in the same group).
  • Cheating the property ordering is not a good idea because it will make your life difficult and the issue will be solved one day.
  • normalize can be replaced by a division.
  • There is no need to check the time as the object is destroyed automatically.
  • Time change should happen after the drawing or the initial state will never be shown.
  • doStepPostEvent should be used instead of doStepPreEvent to avoid a 1-frame delay (more generally for anything rendering related).
  • The draw ellipse action is the same in both cases. It can be moved outside of the conditions to avoid duplication.

Do you have some reservations or questions about them?

@Alios5
Copy link
Contributor

Alios5 commented Nov 23, 2023

@D8H yes regarding doStepPostEvent, it doesn't allow to recover properties like the width of the shape painter object for my gameplay , that's why I thought there was a bug , I'm going to add a checkbox for the user who wants to use it as pre or post event
image

bandicam.2023-11-23.20-54-39-341.mp4

@Alios5
Copy link
Contributor

Alios5 commented Nov 23, 2023

I think it's more user-friendly to have the start and end properties next to each other (in the same group).

When I do this, the end fields are above the start fields.

@Alios5
Copy link
Contributor

Alios5 commented Nov 23, 2023

There is no need to check the time as the object is destroyed automatically.
Cheating the property ordering is not a good idea because it will make your life difficult and the issue will be solved one day.
normalize can be replaced by a division.

Done

@D8H
Copy link
Contributor

D8H commented Nov 24, 2023

yes regarding doStepPostEvent, it doesn't allow to recover properties like the width of the shape painter object for my gameplay , that's why I thought there was a bug , I'm going to add a checkbox for the user who wants to use it as pre or post event

Doing the drawing in postEvent is important to let users change the state in events and have it drawn without 1-frame delay.
Could it make sense to use a progress or a radius instead of the width?
Otherwise, you could disable automatic clearing and do the clearing right before drawing a new frame. This way the object will always have a size.

I think it's more user-friendly to have the start and end properties next to each other (in the same group).

When I do this, the end fields are above the start fields.

There is the same issue in the 3D particle emitter, but GDevelop will keep the property order at some point.

@Alios5
Copy link
Contributor

Alios5 commented Nov 25, 2023

Okay, I'll test it.

@D8H D8H moved this from Needs review to Needs changes in Extensions review Mar 19, 2024
@github-actions github-actions bot moved this from Needs changes to Needs review in Extensions review Apr 9, 2024
@D8H
Copy link
Contributor

D8H commented Apr 9, 2024

Thank you for the changes.

Previous suggestions that weren't addressed:

  • Any enabled event should be uncollapsed.
  • I think it's more user-friendly to have the start and end properties next to each other (in the same group).
  • Cheating the property ordering is not a good idea because it will make your life difficult and the issue will be solved one day.
  • Time change should happen after the drawing or the initial state will never be shown.
  • doStepPostEvent should be used instead of doStepPreEvent to avoid a 1-frame delay (more generally for anything rendering related).
  • In the group "Draw", some actions are the same for "Line" and "Fill" they can be moved outside of the condition to avoid duplication.
  • Actions, conditions and expressions can be generated from the properties.
    • You may need to remove existing actions to be able to select it.
      image

New suggestions:

  • Layer time scale can be handled with this expression: TimeDelta() * LayerTimeScale(Object.Layer()).
  • "Enable fill" may be easier to understand than "Change type".
  • onCreated functions can be used instead of the trigger once.
  • Object.Behavior::PropertyTweenName() can now be written like this TweenName.

@D8H D8H moved this from Needs review to Needs changes in Extensions review Apr 9, 2024
@Alios5
Copy link
Contributor

Alios5 commented Apr 10, 2024

Here's what it looks like when I put them all together
image

@github-actions github-actions bot moved this from Needs changes to Needs review in Extensions review Apr 10, 2024
@D8H
Copy link
Contributor

D8H commented Apr 21, 2024

What I mean is that it's better to have titles for color, shape... rather than start and end.

A bit like the particle emitter:

image

@D8H
Copy link
Contributor

D8H commented Apr 28, 2024

Thanks, almost there!

  • Does Pound mean ponderation? It's not clear. It's the time from the start of the animation. Why not call it Time?
  • ActivateFillshould be renamed IsFilled with something like "Fill the shape" for the label.
    • You may have to adapt the sentences and labels of the action and condition as it's hard for the generator to make correct sentences for boolean properties.
  • You can duplicate the RgbMean from "Color conversion" extension into yours to avoid to deal with color channels directly.
  • TweenName should be renamed Easing ("Name of tween" may be confusing as it sounds like "Tween identifier")
    image
  • "Duration in second" can just be "duration" as there is already the unit on the property field.
  • For every label, there should be an upper-case letter only on the 1st word.
  • About the property groups
    • Rename "Animation scale" to "Timing"?
    • Rename "Angle" to "Rotation"?
    • Move the checkbox into "Outline"?
  • The behaviors should be renamed "EllipseShockWave" and "StarShockWave" because behavior should be nouns not verbs and this update is a breaking change to it's the right time to do it.

@D8H
Copy link
Contributor

D8H commented Apr 29, 2024

I've made an few changes:

  • in names, labels, descriptions and sentences
    • mostly to fix some necessary upper-case
  • in the default property values
    • because users should have something that looks good by default

If you're ok with the changes, please update the PR with this version and it should be ready for a merge into the reviewed list.

DarwShockWaveEffect.zip

@tristanbob
Copy link
Contributor

I did a small test of the version with @D8H changes and everything seemed to work. I'm most happy that users can add the behavior and it works without changing anything. (The old version required them to select "fill" or "line".)

One suggestion: Instead of calling the star "branches", call them "points". I often hear people say "this is a 5-pointed star", or "this star has 5 points". I do not hear people refer to them as branches.

@D8H
Copy link
Contributor

D8H commented May 1, 2024

One suggestion: Instead of calling the star "branches", call them "points". I often hear people say "this is a 5-pointed star", or "this star has 5 points". I do not hear people refer to them as branches.

Indeed, GDevelop draw action and Pixi call them points:
https://pixijs.download/v4.8.0/docs/PIXI.Graphics.html#drawStar

I've made the changes:
DarwShockWaveEffect.zip

I also remove the floor in the formulas to make the animation slightly smoother.

@Alios5
Copy link
Contributor

Alios5 commented May 1, 2024

Add a checkbox for pixel perfect, where the floor will be used for pixel perfect

@D8H
Copy link
Contributor

D8H commented May 1, 2024

Add a checkbox for pixel perfect, where the floor will be used for pixel perfect

What do you mean by "pixel perfect"?
Circles points are not integers even if the radius is.
Have you noticed any artifact that can be solved by rounding values?

If you disable the anti-aliasing, you will see sharp pixels even if the circle radius is not an integer:

image

@Alios5
Copy link
Contributor

Alios5 commented May 1, 2024

ok i tested it it's good as you make it

@Alios5
Copy link
Contributor

Alios5 commented May 1, 2024

You forgot to hide the easing progress! for StarShockwave
image

@D8H
Copy link
Contributor

D8H commented May 2, 2024

You forgot to hide the easing progress! for StarShockwave

Indeed, sorry about that.

Copy link
Contributor Author

github-actions bot commented May 4, 2024

Errors were detected in this submission:

ℹ️ Ignoring validation for extension WithThreeJS.


❌ 1 Error found in extensions - please fix it before generating the registry.

@D8H D8H changed the title Draw shock wave effect [Reviewed] [Shock wave effect] No longer require the Tween behavior May 4, 2024
@D8H D8H merged commit e34a002 into main May 4, 2024
4 checks passed
Extensions review automation moved this from Needs review to Added to GDevelop May 4, 2024
@D8H D8H deleted the extension/Alios5/602 branch May 4, 2024 11:54
@D8H
Copy link
Contributor

D8H commented May 4, 2024

@Alios5 Thank you for the hard work and your patience. It should be in the reviewed extension list in a few hours.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔄 Extension update An update for an existing extension ✨ New extension A new extension 🔍 Reviewed extension An extension that is to be reviewed in great detail before merging.
Projects
Extensions review
  
Added to GDevelop
Development

Successfully merging this pull request may close these issues.

None yet

3 participants