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

Possible tiny optimization in converted particlesmaterial shader #24578

Closed
2plus2makes5 opened this issue Dec 24, 2018 · 3 comments
Closed

Possible tiny optimization in converted particlesmaterial shader #24578

2plus2makes5 opened this issue Dec 24, 2018 · 3 comments

Comments

@2plus2makes5
Copy link

I first made a particlesmaterial and then converted it to a shader and i found some lines in the vertex function that left me perplexed:

...
float pi = 3.14159;
float degree_to_rad = pi / 180.0;
...
float spread_rad = spread * degree_to_rad;
...
CUSTOM.x = base_angle * degree_to_rad;
...

things that don't convince me are:
1)what's the reason of doing "pi/180.0" when we could save the operation by directly assigning the result 0.017453278 (i don't know how many numbers are allowed)? Is it for clarity? A comment would be more clear and wouldn't affect performance.
2)why not simply using the function "radians" to convert degrees to radians? Is it faster to make a simple multiply instead of calling the function? But if so the first division is still a waste.

3)not a issue but for some reason before doing this shader i thought "pi" was a default constant, i think it would be useful to have "pi" constant already defined instead of make us define it every time, that would save an assign and eventual value errors.

Particles are supposed to be many so in my opinion every little saving on their (converted)shader is useful.

Sorry if this is eventually dumb :/
Merry Christmas and happy new year! :D

@clayjohn
Copy link
Member

  1. The compiler optimizes away constant expressions like this. In the compiled code there is no difference between pi / 180.0 and 0.017453278. It is preferable to use pi / 180.0 so that the user can understand where the value is coming from.

  2. I don't think there is any reason not to use radians() here. The compiler would likely optimize it as well, so it would be no different than just multiplying by degree_to_rad. That being said, there is no compelling reason to use it either.

  3. This seems like a good idea. Making it a global constant might actually make it a little faster for users if they use PI a lot. But again, there is not a really compelling reason to add it.

If you really want these changes I suggest making a PR yourself, it wouldn't take a lot to change them and it would be a great way to start contributing!

@2plus2makes5
Copy link
Author

Thanks for the answer!
Sorry if i'm annoying but i'm curious so... It's obvious that i don't know exactly how shaders work, so it's probably a stupid question, but using radians() wouldn't avoid the allocation and assign of pi and degree_to_rad?

@Calinou
Copy link
Member

Calinou commented Sep 1, 2021

The PI constant was added to the shader language by #48837, closing.

@Calinou Calinou closed this as completed Sep 1, 2021
@Calinou Calinou added this to the 4.0 milestone Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants