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

Should use raw string literal in particle_process_material instead of adding statements line by line #85595

Closed
jsjtxietian opened this issue Dec 1, 2023 · 5 comments · Fixed by #89267

Comments

@jsjtxietian
Copy link
Contributor

Godot version

Godot v4.2.beta (5ee9831)

System information

Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 (NVIDIA; 31.0.15.3619) - 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz (16 Threads)

Issue description

As @akien-mga mentioned in #83881 (comment), we can probably do some clean up in particle_process_material.cpp in 4.3

For example, change the code from :

code += "uniform vec3 direction;\n";
code += "uniform float spread;\n";
code += "uniform float flatness;\n";
code += "uniform float inherit_emitter_velocity_ratio = 0;\n";
code += "uniform float initial_linear_velocity_min;\n";
code += "uniform float initial_linear_velocity_max;\n";
code += "uniform float directional_velocity_min;\n";
code += "uniform float directional_velocity_max;\n";

to something like:

code += R"
uniform vec3 direction;
uniform float spread;
uniform float flatness;
uniform float inherit_emitter_velocity_ratio = 0;
uniform float initial_linear_velocity_min;
uniform float initial_linear_velocity_max;
uniform float directional_velocity_min;
uniform float directional_velocity_max;"

Not only it will improve code style, but also more importantly, it can reduce string operation times which can help reduce the shuttering when ParticleProcessMaterial::_update_shader is called.

But while changing this requires all other related pr to rebase on this one, so there is a question about when to do this.

Steps to reproduce

Check particle_process_material.cpp

Minimal reproduction project

N/A

@akien-mga
Copy link
Member

CC @QbieShay

I think this would be a nice change, but indeed we should see when is the right time. This change will make it difficult to cherry-pick changes to 4.2, so would require making dedicated PRs for bugfixes in the particles material for both 4.2 and master. If Qbie is OK with that process, I think we can do the raw string conversion ASAP.

But we can also give it a few weeks first to see if some regressions are reported against 4.2 that need a quick fix. Basically, I would do this change before starting to do more feature work on particles for 4.3.

@QbieShay
Copy link
Contributor

QbieShay commented Dec 1, 2023

Yes I think it'd be nice to wait 2 weeks or so to fix immediate bugs ( i don't think theres any outstanding bug rn though), then this can be done. I'm willing to coordinate but in this period I don't have time to refactor the whole class. Lmk if there's someome willing to do it

@jsjtxietian
Copy link
Contributor Author

Lmk if there's someome willing to do it

IMHO it won't have a huge impact as we just compress the code, so maybe I can help resolve this at the appropriate time.

@QbieShay
Copy link
Contributor

QbieShay commented Dec 1, 2023

ok thank you!

@Calinou
Copy link
Member

Calinou commented Dec 7, 2023

This would be a good opportunity to make this code follow the Godot shader language style guide too, as the current code has many formatting issues (such as not having spaces between function parameters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants