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

Structure proposal #21

Closed
zaksnet opened this issue Jan 7, 2022 · 6 comments
Closed

Structure proposal #21

zaksnet opened this issue Jan 7, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@zaksnet
Copy link

zaksnet commented Jan 7, 2022

You seem to be using the same class (AnimaNode) for everything right but this makes it hard to maintain and possibly prone to errors because you have to pass dictionaries with properties that sometimes are not compatible with each other. Plus users will have to remember all the names of the properties they have to pass. A better approach would be to have a different class for each type of animation. An example would be:

# Create a class named AnimaAnimationGrid (or something similar)
# then pass the type of the animation into the Begin method so this would return an AnimaAnimationGrid node instead of a generic AnimaNode.
# With that in place now you can have proper properties for the methods instead of dictionaries (kind of like tween but with all of the nice features your project provieds)
var anima := Anima.Begin(AnimaAnimations.Grid, self)
anima.then(
   grid: $Grid,
   grid_size: grid_size,
   animation_type: Anima.GRID.COLUMNS_EVEN,
   property: "x",
   to: 5, 
   relative: true,
   duration: 0.3,
   easing: Anima.EASING.EASE_OUT_SINE,
   items_delay: 0
)

That way users wont have to go back to the documentation in order to check which parameters they can pass etc...

EDIT: And of course have a base class from which all of the animations inherit

@zaksnet zaksnet changed the title Structure proposan Structure proposal Jan 7, 2022
@ceceppa
Copy link
Owner

ceceppa commented Jan 7, 2022

I think it would be impossible to mix different animations, such as animating a single node and a group using the same Anima.

Also, I did avoid on purpose parameters and preferred a Dictionary because Godot does not support named parameters, and you have to pass all the parameters, even if not used.

Said that we could use a different syntax that is more dev friendly, for example:

var anima := Anima.begin(self)

anima.then(
	Anima.Node($Control) \
	.anima_property("position") \
	.anima_duration(0.3)
)

anima.then(
	Anima.Grid($Grid) \
	.anima_grid_size(Vector2(3, 2))
)

The then, with and also function will accept one of the following classes:

  • Anima.Node - to animate a single node
  • Anima.Group - to animate a group of nodes
  • Anima.Grid - to animate a grid of nodes

By doing so, the editor will only show you the methods that exist for the type of Animation you're creating.
Also, the prefix anima_ helps to filter out all the default class methods and properties:

Screenshot 2022-01-07 at 15 23 39

@ceceppa ceceppa added the enhancement New feature or request label Jan 7, 2022
@Terria-K
Copy link
Contributor

Terria-K commented Jan 7, 2022

You seem to be using the same class (AnimaNode) for everything right but this makes it hard to maintain and possibly prone to errors because you have to pass dictionaries with properties that sometimes are not compatible with each other. Plus users will have to remember all the names of the properties they have to pass. A better approach would be to have a different class for each type of animation. An example would be:

# Create a class named AnimaAnimationGrid (or something similar)
# then pass the type of the animation into the Begin method so this would return an AnimaAnimationGrid node instead of a generic AnimaNode.
# With that in place now you can have proper properties for the methods instead of dictionaries (kind of like tween but with all of the nice features your project provieds)
var anima := Anima.Begin(AnimaAnimations.Grid, self)
anima.then(
   grid: $Grid,
   grid_size: grid_size,
   animation_type: Anima.GRID.COLUMNS_EVEN,
   property: "x",
   to: 5, 
   relative: true,
   duration: 0.3,
   easing: Anima.EASING.EASE_OUT_SINE,
   items_delay: 0
)

That way users wont have to go back to the documentation in order to check which parameters they can pass etc...

EDIT: And of course have a base class from which all of the animations inherit

This kind of style syntax isn't possible with GDScript yet.. as GDScript doesn't support Structs. This only be possible in C# as it has a structs. Yes, I agree about hard to maintain a property and had to look on the documentation, but there are many parameters that this addon has in just one function.

@Terria-K
Copy link
Contributor

Terria-K commented Jan 7, 2022

I think it would be impossible to mix different animations, such as animating a single node and a group using the same Anima.

Also, I did avoid on purpose parameters and preferred a Dictionary because Godot does not support named parameters, and you have to pass all the parameters, even if not used.

Said that we could use a different syntax that is more dev friendly, for example:

var anima := Anima.begin(self)

anima.then(
	Anima.Node($Control) \
	.anima_property("position") \
	.anima_duration(0.3)
)

anima.then(
	Anima.Grid($Grid) \
	.anima_grid_size(Vector2(3, 2))
)

The then, with and also function will accept one of the following classes:

  • Anima.Node - to animate a single node
  • Anima.Group - to animate a group of nodes
  • Anima.Grid - to animate a grid of nodes

By doing so, the editor will only show you the methods that exist for the type of Animation you're creating. Also, the prefix anima_ helps to filter out all the default class methods and properties:

Screenshot 2022-01-07 at 15 23 39

I think this is great, maybe if it's possible to not have a prefix?

@ceceppa
Copy link
Owner

ceceppa commented Jan 7, 2022

Yeah, I was thinking of the prefix only because the autocomplete shows too many options by default.
Happy to don't have it :)

@Terria-K
Copy link
Contributor

Terria-K commented Jan 7, 2022

Yeah, I was thinking of the prefix only because the autocomplete shows too many options by default. Happy to don't have it :)

Oh.. it seems prefix will be a good idea then. You could let that stay instead.

@ceceppa
Copy link
Owner

ceceppa commented Jan 10, 2022

I've pushed an initial implementation for this. The then and with methods now accept a class as a parameter, and the built-in ones are:

  • Anima.Node
  • Anima.Group
  • Anima.Grid

Possible to pass a dictionary is still possible tho

ceceppa added a commit that referenced this issue Jan 10, 2022
@ceceppa ceceppa closed this as completed Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants