craftworkgames / MonoGame.Extended

Extensions to make MonoGame more awesome
http://www.monogameextended.net/
Other
1.43k stars 324 forks source link

Particle rework #162

Closed ExCellRaD closed 8 years ago

ExCellRaD commented 8 years ago

Because the particle implementation comes from a different framework, it's not integrated too much into this one. I would like to change that, while mantaining the current functionality.

Main changes:

More shape changes:

Could you open a new branch for this, particle-rework perhaps?

lithiumtoast commented 8 years ago

Get rid of all the unsafe and pointer stuff, for consistency accross the framework.

The unsafe code and pointers is so it can run faster. For example, iterating an array in safe code has to check if the index is out of range every time the array is accessed with an index. With unsafe code you can skip that redundant check and reap the benefits it brings to performance. MonoGame just recently switched to unsafe code for their SpriteBatch for exactly this reason.

More shape changes:

While we are on this subject, I would like to have a way to get the vertices of a shape for drawing. For example, going from a CircleF to an array of VertexPositionColor[], or any IVertexType with a position attribute. A re-design of the shapes probably belongs in it's own issue.

New base interface (IShapeBase) shared by IShapeF and IPathF to expose the same methods.

I don't think a path should have it's own interface. It should be a struct or a class type.

PointF is a shape wrapper for Vector2

Why have a PointF then? Why not just use Vector2 as a point?

Curves

Bezier curves would be cool.

ExCellRaD commented 8 years ago

The unsafe code and pointers is so it can run faster.

Didn't know this, will leave it in then.

A re-design of the shapes probably belongs in it's own issue.

These changes all directly impact/are needed for the particle changes, more changes can come after this one is finished since this appears to be a short one without removing the unsafe code.

Why have a PointF then? Why not just use Vector2 as a point?

because I can't add interfaces to Vector2 and can be used as a shape in many ways

I don't think a path should have it's own interface. It should be a struct or a class type.

The interface is because LineF implements it as well, and I didn't want to make it behave as a curve. Plus it wouldn't be a struct anymore if it wasn't an interface. Would a better solution be to just implement IShapeBase and leave the shared properties between LineF and curves separate or make a shared base class?

lithiumtoast commented 8 years ago

The current shapes are structs which just implement the interface IShapeF, like CircleF. Care has to be taken not to box the structs if trying to cast to IShapeF. I think the purpose of using structs was to have it like Rectangle which is also a struct.

The interface is because LineF implements it as well, and I didn't want to make it behave as a curve.

A curve and a line could have it's own separate struct / class.

it wouldn't be a struct anymore if it wasn't an interface.

I'm not sure what you are saying? A struct and a class can both implement an interface. Boxing happens when casting the struct to an interface though, unless a generic parameter is used on a method or class for the interface. See http://stackoverflow.com/questions/4403055/boxing-unboxing-and-generics

ExCellRaD commented 8 years ago

I was talking about LineFinheriting the class Curve, which would mean LineF would not be a struct anymore. Perhaps we should open a new issue for discussion about the shapes? There's probably a lot of stuff you want to implement as well (for collisions/meshes).

craftworkgames commented 8 years ago

Hey guys.

I agree that the particle code could be integrated better into the library. I did a little bit of this when I pulled it over, but not everything.

I think most of the main changes you've proposed are okay. We just have to be careful to fully understand them though.

I'm not 100% convinced an Axis is the same as a Vector2. In the original code there was a Vector struct that I turned into a Vector2 but I think it was always intended to keep the Axis as separate type. Besides, there's already an implicit conversion.

Get rid of all the unsafe and pointer stuff, for consistency accross the framework.

Do not do this. Consistency is a nice thing to have in a public API but it's rarely a good reason to change the implementation details. As a rule of thumb, consistency should not be the only reason to change something.

As @LithiumToast outlined above, the unsafe code is here for a good reason.

Perhaps we should open a new issue for discussion about the shapes? There's probably a lot of stuff you want to implement as well (for collisions/meshes).

Yes, it feels like we should take the shapes discussion elsewhere and maybe implement those changes first. The discussion about the interfaces isn't really clear to me yet.

I'll create the particle-rework branch now.

ExCellRaD commented 8 years ago

I'm not 100% convinced an Axis is the same as a Vector2.

To state it blunt: it's a Vector2, but normalized. The only usages I've found in the code was to use the constructor to turn a vector into a normalized version of itself. The way it gets used makes it seem like an internal wrapper for a vector that describes a direction (which is one of the main functions of a vector already). Since the only usages that get removed are the particle spawners (the rest works just the same with Vector2), we just have to make sure that the heading always gets normalized before being returned. I think it would be a smart move to remove the need for Axis, as it will increase performance at high particle counts and it would be a tighter integration.

craftworkgames commented 8 years ago

as it will increase performance at high particle counts and it would be a tighter integration.

Woah, let's not make assumptions about how it will impact performance without measuring it first. From what I've seen so far the particle system is already very well optimized by the original author, I'd be surprised if he missed something so obvious.

I'm not saying we shouldn't change an Axis to a Vector2 but I am curious what that brings to the table.

My guess is that whatever we do answers the above questions slightly differently. It's not always a simple "we should definitely do it" answer.

ExCellRaD commented 8 years ago

as it will increase performance at high particle counts and it would be a tighter integration.

I believe it will increase it slightly because of conversions to Vector2every frame, but yes you are right, I haven't tested it.

Does it make the code easier to understand?

For people who understand vectors already, I think so. People who don't probably need to understand vectors first anyway.

Does it reduce the amount of duplicated code?

Slightly, we can remove a whole class which has functionality that already exists in Vector2 it also means that theres less 'clutter' when using the framework (one less option when using Extended.Particles).

Does it change the performance of the code?

It might have a small increase in performance, albeit very small probably, I'm going to test this.

Does it break the current API?

As the axis struct doesn't get used anywhere outside of the particle, removing it shouldn't be a problem. For internal (in particles) uses, functionality will stay the same. The only thing I can see 'breaking' is that people implementing custom Profile classes will need to make sure to normalize the vector. We could circumvent this by normalizing the heading parameter after getting it from the Profile. This would even allow for a more clear implementation in my opinion.

Would it really matter

Not really, it's actually a very small change. Combining all of the reasoning used above, I just don't see a reason to keep it.

craftworkgames commented 8 years ago

Okay, fair enough. The only thing that irks me a little is the need for people to normalize it themselves. If we can get around that it's probably okay.

I'm still curious why the original author did it that way. The devil might be in the details.

ExCellRaD commented 8 years ago

conversions to Vector2 every frame

This was a mistake, it's only when getting the heading from the Profile or when using some modifiers. particle->Velocity = heading * speed; This gets executed every time a particle spawns. If the heading is an Axis, it gets converted to a Vector2 (which it allready is because it's not normalize anymore when multiplying with the speed). If heading is a Vector2 there's no need for the conversion.

craftworkgames commented 8 years ago

If the heading is an Axis, it gets converted to a Vector2

Removing this conversion could be the best reason we'll talked about so far to remove the Axis class. At the very least I'm curious about the impact of doing it.

Jjagg commented 8 years ago

If you need any help or have any questions about this, feel free to ask. (I'm not the original author, but I played around with it a bit and added to it for use with MonoGame)

ExCellRaD commented 8 years ago

If you need any help or have any questions about this, feel free to ask.

Not many questions really, but more looking for cool ways to make the particle system to have more functions. Currently I'm thinking about making it more modular. For example you have the current velocity modifier which changes the color depending on the speed of the particle, I'm adding the option to change whatever property you want, not only the color. Also I'm looking into having the constraining modifiers use shapes to define areas, might need some insight into how to reflect particles back once they have bounced (I don't know if you could help me out with that?). But as I said, looking for more things like this, so feel free to drop any suggestions/ideas :)

Jjagg commented 8 years ago

Yeah, it could do with some more modularity :) For the reflection, you can probably get away with reversing the velocity vector and mirroring along the normal of the crossed edge, but if you'd want it to work perfectly in all cases i guess you'd have to do it iteravely. I've played around with stuff like this before, so i could definitely help. About the features, after working on this engine I worked a bit on noise generation. Specific kinds of randomness could create some cool patterns for a particle engine I think. Also in general a noise library would be cool for MGExtended (procedural generation, yay), but that should be discussed in a seperate issue.

Jjagg commented 8 years ago

I'm also still interested in writing a visual editor for the engine, so if I ever get around to it, I'll do it for this version since its probably the most used. I had a pretty much working editor done in winforms, but IMO it should be cross platform.

ExCellRaD commented 8 years ago

noise generation

I have no idea how that works, it sounds cool though :p

I'm also still interested in writing a visual editor for the engine

That would be awesome, it could even be the start of a full game engine editor using MGExtended.

I'm going to make a pull request with changes I already have, you can take a look there or here

Jjagg commented 8 years ago

full game engine editor using MGExtended

Not sure if that's a goal for MonoGame.Extended. @craftworkgames? Some standalone tools could be very useful though.

craftworkgames commented 8 years ago

I'm adding the option to change whatever property you want, not only the color.

Could you provide an example of how this would be used a real game? What properties would you change for what kind of effect?

Also in general a noise library would be cool for MGExtended (procedural generation, yay), but that should be discussed in a seperate issue.

Yeah, that does sound interesting as a separate feature. Raise an new issue if you have ideas.

I'm also still interested in writing a visual editor for the engine, so if I ever get around to it, I'll do it for this version since its probably the most used. I had a pretty much working editor done in winforms, but IMO it should be cross platform.

A particle editor would be cool. LibGDX has one if you want some more inspiration. Eto forms might be a good way to make it cross platform.

it could even be the start of a full game engine editor using MGExtended.

Try to think if MonoGame.Extended as a collection of small libraries that work well together. I know the difference is subtle but it's not really a game engine. I strongly believe a lot of people use MonoGame because they don't want to be restricted by the confines of a fully fledged game engine. It's important that people can cherry pick the bits of the library they want and ignore the rest.

This vision should carry over to the tools. I think it's a great idea to have a bunch of tools that work well with MonoGame.Extended but I don't want to build the next Unity. We favor stand alone tools over one "do it all" tool.

ExCellRaD commented 8 years ago

Could you provide an example of how this would be used a real game?

I'm really bad at that :p, I just try to add a lot of functionality to leave it up to the people using it. The properties would be Color, Rotation, Mass, Scale and Opacity. Maybe something like fading out particles as soon as they enter an area defined by a shape? Making particles have less mass the faster they go?

Jjagg commented 8 years ago

I'll check out my previous attempts at a noise library. I actually have 2 lying around :) One for CPU, but it was horribly slow (I think because of cache misses) and one for GPU that was pretty cool to make with all the shaders and stuff, but it wasn't as flexible as I'd like because you're working with textures rather than mathematical formulas. So I'll probably try another approach. I have a fair idea what should be in it now. I think i'll start working on it this week and will open an issue and/or pull request when i got something.

ExCellRaD commented 8 years ago

I'll close this because most of changes are either already done or wont get done. There can be shape-based modifiers in the future, but we can make a separate issue for that then.