flame-engine / flame

A Flutter based game engine.
https://flame-engine.org
MIT License
9.29k stars 913 forks source link

Rename Animation Class #419

Closed obitodarky closed 4 years ago

obitodarky commented 4 years ago

Flutter has it's own Animation class in Material Library which is very conflicting while using Flame's Animation class. IDE doesn't suggest importing the Flame's Animation class but instead takes Flutter's class as Default, even after importing, the IDE still throws an error that problems will occur while using classes with the same name.

Screenshot 2020-07-15 at 7 27 58 PM

There is indeed a workaround to use as to avoid this.

import 'package:flame/animation.dart' as animation;

...
//use this for Animation class of Flame
animation.Animation;
...

However, we should change the name of the class to make it simpler for Developers.

obitodarky commented 4 years ago

I'd love to work on this issue @luanpotter @renancaraujo :)

erickzanardo commented 4 years ago

This discussion has been raised already and we don't see the need to rename this.

Yes there has that name clash, but this is not the only one that can clash and since the Flutter framework has a lot of classes and features, it is unlikely that we can prevent all the name clashes.

Using the as on import is not a workaround IMO, it is supposed to be used exactly on situations like this.

And this should only happens when you need to use the animation from flame, on the same file that you are using flutter widgets, which is most likely to only happen on small projects that are not organised with components and entities.

So I don't think we should do this rename, @luanpotter @renancaraujo @spydon, what are your thoughts?

obitodarky commented 4 years ago

@erickzanardo I guess that kind of makes sense, but since Flame Components are very independent, people who are new to it and learning it may find it frustrating. I myself was just trying to play around with Flame and this happened, I couldn't figure out what the problem was because the Animation class I used didn't show any error at all, rather just told me that there was no such message for the class. I'm sure new developers will have a tough time with this.

Since we don't want to break the library for people who are using it right now, we can create a duplicate of the Animation class for example, FlameAnimation while keeping the original Animation. When developers are using the Animation class, we can show a warning that they should use FlareAnimation instead and the old Animation class will be deprecated in 3-4 months. That way, we can prepare current developers and avoid hassles and confusion. We can do this for other widgets as well.

erickzanardo commented 4 years ago

@obitodarky even though I am not sure about the renaming, we did an extensive discussion with the tema ad @renancaraujo convinced us that SpriteAnimation is a better name, as it gives more context about what the animation is about.

We will do that for the v1 since this is a total breaking change.

Thanks for raising that discussion @obitodarky

obitodarky commented 4 years ago

@erickzanardo that's great SpriteAnimation seems a viable name. I'd really love to be involved in this issue, is there any way I can help?

erickzanardo commented 4 years ago

@obitodarky, @renancaraujo is going to handle that issue, if you two can find a way to collaborate it would be awesome, if you are not yet, I suggest you join our discord, so you two could communicate easier.

erickzanardo commented 4 years ago

Done on v1.0.0