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

Particle-updates #196

Merged
merged 10 commits into from
Feb 23, 2023
Merged

Particle-updates #196

merged 10 commits into from
Feb 23, 2023

Conversation

keianhzo
Copy link
Contributor

This PR fixes the particle-emitter defaults so they are aligned with the client ones and adds a gizmo.

@keianhzo
Copy link
Contributor Author

keianhzo commented Feb 1, 2023

I'll need to fix the tests, is good to see that they are useful though :)

@Exairnous
Copy link
Contributor

Nice! I really like the gizmo model and I think making it more consistent with Spoke is great. I have a couple notes/ideas though.

  • The Z end velocity is still 0.0 (should be 0.5)
  • The image src doesn't match Spoke's.
  • The velocity properties aren't getting Y and Z swapped on export.
  • It would be nice to have X, Y, and Z labels on the velocity sliders.
  • The properties aren't in the same order as in Spoke. This may not matter if we get some kind of preview, but in the mean time I think it's common practice to setup a particle emitter in Spoke and then just copy the settings over, so having the property order match Spoke would make this much easier.
  • Adding a 2x2m plane to the gizmo facing down -Y would be very helpful to visualize the area that the particles will cover.
  • A nice touch might be having the gizmo color match the start color property, the same as is done for light gizmos. (Ideally the bottom particles would have the starting color, the middle particles would have the middle color and the top particles would have the end color, but I don't think the current gizmos will allow that)
  • To give a good idea of the particles' direction and area, I think ideally the 2x2m plane would match the object rotation and scale to show the area, while the "particle" part of the gizmo would rotate to match the start velocity and show the direction the particles will flow in (at least to begin with). This may require two gizmos on the same component (for our current system), so it may be a fair amount of work to implement, so I'm not sure if you want to try adding it to this PR, but I thought I'd throw the idea out there :)

@keianhzo
Copy link
Contributor Author

keianhzo commented Feb 6, 2023

The velocity properties aren't getting Y and Z swapped on export.

I'm surprised that nobody has reported this in 6 moths. Maybe they are too annoying to use in Blender and nobody really does or maybe people just get used to swapping axis.

@Exairnous
Copy link
Contributor

I'd guess it's probably a bit of both, plus I'm not sure particle emitters are as regularly used as other components.

Now that the axes are getting properly switched, I think there should be a version bump/migration for the component, the same as we did for media frames. And we may also want to switch the 0.5 velocities to the Y axis so that the end result matches the behavior in Spoke.

Also, it looks from my testing that the 2x2m plane you added to the gizmo should be rotated 90 degrees on the X axis. But I'm not sure what to do about the particle spray though, since the particle direction is determined by a combination of the object rotation plus the velocity values, it should theoretically be rotated separately from the plane, but I don't think that's really an option right now; so maybe for the time being it should keep its upwards orientation, but move it down and center it in the plane so it forms a kind of emblem?

@keianhzo
Copy link
Contributor Author

I agree that showing the velocity on the gizmo should be very neat but I'm going to keep it simple for now and just add this improvements. We can do a pass sometime on making the gizmos a bit more fancy.

@keianhzo keianhzo merged commit 26c7110 into master Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants