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

Stop Particles3D to use ressources when not emitting (fixes #19507) #20338

Closed
wants to merge 1 commit into from

Conversation

malbach
Copy link
Contributor

@malbach malbach commented Jul 21, 2018

This PR prevents Particles3D to use resources when not emitting (issue #19507).

For the record, this does the same as my last PR #19764, but this time internally.

@malbach malbach requested a review from reduz as a code owner July 21, 2018 20:41
@akien-mga
Copy link
Member

Could you rebase to squash the commits together?

@malbach
Copy link
Contributor Author

malbach commented Jul 21, 2018

@akien-mga done (sorry, forgot to branch-away 😖)

@malbach
Copy link
Contributor Author

malbach commented Jul 21, 2018

@reduz or anyone else who can answer:
I have made another modification for Particles3D (stop redrawing viewport when inactive) but I prefer to RFC before PR 😊
Here's the diff : godot.diff.txt

What do you think ?

@vnen
Copy link
Member

vnen commented Jul 22, 2018

I prefer to RFC before PR

Well, PR is RFC. Also much easier to review using GitHub interface.

@malbach
Copy link
Contributor Author

malbach commented Jul 22, 2018

@vnen ok 👍

@malbach malbach force-pushed the godot_malbach branch 5 times, most recently from 4a462e1 to b98a0b5 Compare July 27, 2018 12:27
reduz
reduz previously requested changes Jul 29, 2018
@@ -1410,6 +1410,9 @@ void RasterizerSceneGLES3::_setup_geometry(RenderList::Element *e, const Transfo
RasterizerStorageGLES3::Particles *particles = static_cast<RasterizerStorageGLES3::Particles *>(e->owner);
RasterizerStorageGLES3::Surface *s = static_cast<RasterizerStorageGLES3::Surface *>(e->geometry);

if (!particles->emitting && particles->inactive)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too low level, I think you should be able to stop the node from being added to render list by simply remove it it where you check in VisualServerScene. Check around that code, there is a bool you can toggle to get it removed

@@ -1796,6 +1796,8 @@ void RasterizerSceneGLES3::_render_geometry(RenderList::Element *e) {
}
}

VisualServerRaster::redraw_request();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is needed here

@@ -1886,7 +1886,7 @@ void VisualServerScene::_prepare_scene(const Transform p_cam_transform, const Ca
//particles visible? process them
VSG::storage->particles_request_process(ins->base);
//particles visible? request redraw
VisualServerRaster::redraw_request();
//VisualServerRaster::redraw_request();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an ok place to avoid it from rendering if inactive, you can remove it from the render list

@malbach malbach force-pushed the godot_malbach branch 4 times, most recently from 0f39ca2 to 9970340 Compare July 31, 2018 08:16
@malbach
Copy link
Contributor Author

malbach commented Jul 31, 2018

Made some changes, in fact I went back to my first modification but at the time I thought it was too simple, and looked for something... "better"
This PR also reverts #19682, as it is not needed anymore

@malbach malbach force-pushed the godot_malbach branch 4 times, most recently from 4fa642a to 8ef916a Compare August 6, 2018 16:15
@malbach malbach requested a review from karroffel as a code owner August 6, 2018 16:15
@akien-mga akien-mga requested a review from reduz August 14, 2018 14:21
@malbach malbach force-pushed the godot_malbach branch 2 times, most recently from a4faedb to 79eb777 Compare August 24, 2018 13:40
@malbach
Copy link
Contributor Author

malbach commented Oct 22, 2018

Hi,
After seeing this I think my PR isn't needed anymore :)

@malbach malbach closed this Oct 22, 2018
@malbach malbach deleted the godot_malbach branch January 13, 2020 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants