neoforged / NeoForge

Neo Modding API for Minecraft, based on Forge
https://projects.neoforged.net/neoforged/neoforge
Other
1.17k stars 172 forks source link

Be able to disable particle culling in the Forge configuration file #189

Closed Lolothepro closed 2 months ago

Technici4n commented 11 months ago

What is the point of disabling an optimization, assuming the bugs with it are fixed?

Lolothepro commented 11 months ago

What is the point of disabling an optimization, assuming the bugs with it are fixed?

If "alwaysSetupTerrainOffThread", the Forge expert pipeline have a setting to disable it, it might be a good idea to also make a setting to disable particle culling.

Lolothepro commented 11 months ago

then, if it's too complicated to do, you shouldn't do it at all.

Technici4n commented 11 months ago

I get it but it would be nice to have a more fleshed out reasoning to make sure that we understand if someone depends on disabling this or if this is just a low priority "nice to have"...

(BTW, the 45° block fix is not a NeoForge setting, and should not have been a Forge setting.)

Lolothepro commented 11 months ago

a low priority "nice to have"

XFactHD commented 11 months ago

If "alwaysSetupTerrainOffThread", the Forge expert pipeline or the 45° block fix have a setting to disable it, it might be a good idea to also make a setting to disable particle culling.

Those three are all bad examples, alwaysSetupTerrainOffThread is a leftover from before the existence of vanilla's "Chunk Builder: Threaded" option and is as far as I can tell entirely superseded by that, the experimental pipeline is not an optimization and, as Tech mentioned, the 45° stuff doesn't have and shouldn't have a config setting.

sciwhiz12 commented 11 months ago

I see no good reason to provide a configuration option for something which (outside of bugs) should never be visible/felt by a user, as particle culling happens on out-of-view particles (to my knowledge).

embeddedt commented 5 months ago

I'd like us to revisit this issue and make a final decision. Below I sketch my personal reasoning around the problem. This should not be taken as an official stance of NeoForge.

In general, I suspect the average modded world will fall into one of the following two categories:

In the former case, culling should bring a benefit by definition, since even with very rough estimates, it's obvious the player cannot see at least half the world (anything behind them), and culling shouldn't double particle render time. Therefore, there is a benefit to culling the particles over always rendering them.

In the latter case, the particles are likely to only be visible when looking in a certain direction. When not looking in that direction, culling has been tested and confirmed to be a performance benefit as of 1.20.4 (XFactHD tested and saw roughly 4x increase).

The only outstanding concern is therefore the case where the player is actually looking in the direction of those clustered particles. During that time, culling is by definition a regression, since the check will be performed and always return that the particle needs to be rendered. However, it's important to consider that the player is not actually looking in one direction most of the time during regular gameplay. Thus, as long as they are looking away from that location at least 50% of the time, the average performance will still be better, for the same reasoning as in the first paragraph.

We haven't seen any profiling data that suggests particle culling has been a major bottleneck in the past, and as written above, it seems more likely that it would help rather than hinder performance in most real-world scenarios.

Therefore, I don't see the merit in adding an option. I'm happy to be corrected on any of my points above if there's something I didn't take into account. :slightly_smiling_face:

sciwhiz12 commented 5 months ago

The above opinion by @embeddedt is not the final decision of the team. This should be left up for some time to gain the opinions of others on the team, and of the community.

If there are no compelling arguments to add an option after such time, we can close this issue then.

TelepathicGrunt commented 2 months ago

Closing as enough time as has passed and no one argued for this config option