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

Unused variable in gpu_particles_3d_editor_plugin.cpp #43643

Closed
Tracked by #61067
qarmin opened this issue Nov 18, 2020 · 12 comments
Closed
Tracked by #61067

Unused variable in gpu_particles_3d_editor_plugin.cpp #43643

qarmin opened this issue Nov 18, 2020 · 12 comments

Comments

@qarmin
Copy link
Contributor

qarmin commented Nov 18, 2020

Godot version:
4.0.dev.custom_build. dd54851

Issue description:
image2 is assigned but never used anywhere
also point_img2 is unused, because result is used only in image2

Ref<Image> image2 = memnew(Image(w, h, false, Image::FORMAT_RGBF, point_img2));
Ref<ImageTexture> tex2;
tex2.instance();
material->set_emission_normal_texture(tex2);
} else {

also image variable is not used

Ref<Image> image = memnew(Image(w, h, false, Image::FORMAT_RGBF, point_img));
Ref<ImageTexture> tex;
tex.instance();
Ref<ParticlesMaterial> material = node->get_process_material();

@2002Bishwajeet
Copy link

2002Bishwajeet commented Nov 18, 2020

I am newbie here. I want to contribute here . can I work on this??
Can you Also help me out how to work on it???

@qarmin
Copy link
Contributor Author

qarmin commented Nov 18, 2020

I'm not sure if junior job tag is proper.
Basing on logic, this images should be used(code have 4 years, so author probably doesn't remember what exactly should this look like).
Probably code should looks that

tex.instance();
tex.create_from_image(image);

and a little lower

tex2.instance();
tex2.create_from_image(image2);

You can open PR with this fixes

@namedtoaster
Copy link

namedtoaster commented Nov 19, 2020

@qarmin shouldn't it be

tex->create_from_image(image);

and

tex2->create_from_image(image2);

?

I'm just starting contributing here, so I'm curious what this accomplishes? If that's all that needs to be done, I can submit a PR.

Edit: just finished compiling, gonna dig to see if I can figure it out

@qarmin
Copy link
Contributor Author

qarmin commented Nov 19, 2020

I don't even know when to use . or -> because QtCreator itself inserts the correct character (in this case indeed ->).

I don't know this code very well, but it seems to calculate and set the texture of the emission.

The problem is that there is an unused variable, the removal of which would create another unused variable and would make it necessary to remove most of the code from the function.

Since this function seems to make sense, the only solution will be to link the data inside through the create_from_image function.

This should be enough, so PR can be created with this changes(someone more familiar with the code should check PR soon).

@akien-mga
Copy link
Member

To clarify, the current code does nothing due to the lack of calls to tex->create_from_image(image);. Well not nothing, but it assigns empty textures instead of the images created before.

So to fix this, there's two options:

  • Add the calls to tex->create_from_image(image); to actually fill the textures. But then you need to actually see where this code is used in the editor and see if those textures are actually meant to be used, or if this bug which has never worked is meant for the trash can.
  • Remove all the unused code as the current editor seems to work without it, so this might have been WIP that was discarded.

To decide, the best is to do the first option, use the GPUParticles3D editor plugin, see what changed and if that's good or bad.

The code was written by @reduz, but @JFonS may be able to tell if it's needed or not.

@namedtoaster
Copy link

Got it. Well I can wait for @JFonS or @reduz reply. Until then I’ll continue investigating

@JFonS
Copy link
Contributor

JFonS commented Nov 19, 2020

I would say it's just missing the create_from_image calls. But tomorrow I will double-check the rest of the code does what it's meant to do.

@JFonS
Copy link
Contributor

JFonS commented Nov 20, 2020

The create_from_image calls are present in 3.2, and it looks like @reduz removed them in 3f335ce. We should investigate whether we can just add them back or there is an alternative way to achieve the same result.

@gxhamster
Copy link

I dont think that @reduz removed that.
I saw calls to create_from_image in his PR

@JFonS
Copy link
Contributor

JFonS commented Feb 6, 2021

@gxhamster If you check the commit I mentioned (3f335ce), in the editor/plugins/particles_editor_plugin.cpp file, the create_from_image() calls were removed.

It's probably just a mistake in the repetitive task of updating all create_from_image() calls, but someone needs to test if adding them back breaks anything.

W4RH4WK added a commit to W4RH4WK/godot that referenced this issue Mar 4, 2021
…tor::_generate_emission_points

Images for emission point texture and normals were created, but not
transferred to textures.

fix godotengine#43643
@and-rad
Copy link
Contributor

and-rad commented Jul 20, 2022

As far as I can see, this issue and its related PR can be closed. The calls to create_from_image() were restored during the refactor to static methods for image creation (#60739).

@Calinou
Copy link
Member

Calinou commented Jul 20, 2022

Closing per @and-rad's comment.

@Calinou Calinou closed this as completed Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
8 participants