Add a cloud renderer that uploads geometry to the GPU, rendering much faster than vanilla.

[Zaggy1024]

In the profiler, this new renderer gets ~0.47% of gameRenderer time when I have a 16 chunk render distance, as opposed to vanilla clouds' ~9-11%.

It may be preferable to make this use WorldProvider.cloudRenderer instead, but I wasn't sure where to register the renderer class from to handle WorldEvent.Load (assuming this is the best way to set cloudRenderer).

[Zaggy1024]

Updated to use an enum singleton and have a config entry to disable the feature.

Side note, this renderer should work on all hardware that vanilla Minecraft works on, so if you test it and it doesn't work, let me know.

[diesieben07]

Why in the world is this a for-loop? Just write the case statements one after the other with an if around the 3rd one. This is just... confusing.

[asiekierka]

I feel like this should be a separate mod, while Forge should only have the necessary hooks.

(Also, target the 1.9.4 branch, please.)

[Zaggy1024]

I'll leave the decision on whether to include it in Forge up to Lex and fry, but I feel that having some of the most extreme render time hogs fixed would be a big benefit. Some people on IRC have noted they always turn the cloud setting to fast because the fancy clouds are too slow, but this makes fast and fancy clouds perform almost exactly the same. It may also be worth noting that this implementation is a bit better than Optifine's, since Optifine just updates a display list every 20 ticks with injected hooks around the vanilla renderer (from what I could tell).

Indeed, it hadn't occurred to me to target the 1.9.4 branch. Is 1.9.4 in a state where that would be a good thing to do?

[asiekierka]

Absolutely.

[Actuarius]

@williewillus added labels [Performance]

[mezz]

Master is what you should target, it's merged with 1.9.4 now.
This should be updated to resolve the conflicts.

[mezz]

This doesn't look like it's left here intentionally

[Zaggy1024]

Fixed the issues noted by @mezz and @kashike, and the branch is now up to date and ready to merge.

[williewillus]

Does this actually replace vanilla clouds if enabled? Where does it do that, I must've missed it?

[Zaggy1024]

Whoops, forgot to run genPatches. :|

Edit: Gonna take a bit, fresh workspace.

[Zaggy1024]

Done. :)

[williewillus]

Rebase to 1.10.x @Zaggy1024? Would love to see this in

[Zaggy1024]

I'd rather have confirmation that this type of change is appropriate in Forge before I put more work into it. (I haven't touched modding in quite a while.)

[LexManos]

The main problem is that you havent given any performance statistics or profiler data to justify these changes.
I have no idea if its possible for rendering things like this, but it'd be good if you could throw a test suite together in the test directory that actually profiles the two different methods and gives actual, reproduceable stats.

[asiekierka]

I'm still of the opinion this should be a separate mod. I do not think a
rewrite of the cloud renderer belongs in a compatibility layer/mod loader
that Forge essentially is.

2016-08-13 0:14 GMT+02:00 LexManos notifications@github.com:

The main problem is that you havent given any performance statistics or
profiler data to justify these changes.
I have no idea if its possible for rendering things like this, but it'd be
good if you could throw a test suite together in the test directory that
actually profiles the two different methods and gives actual, reproduceable
stats.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2872 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAG7au0Jk2QhQguoPYGy9C53Enbh0wX1ks5qfPA4gaJpZM4IilbO
.

[RANKSHANK]

^So encourage more community driven ASM usage on high traffic classes?

@Zaggy1024 original post says ~0.47% v ~9-11% game render time. That's quite substantial if it can be proven. Try doing what mezz did here in #3093: standardized test scenario, and outcome for standard forge and modified forge.

[LexManos]

@asiekierka your opinion doesn't matter.
This is a simple change matanance wise. Which honestly, it it goes in Forge will most likely be eaten by vanilla in a version or two.
When it comes to these types of changes, this guy is actually doing it perfectly.
Smallest possible patch to the base classes, simple configuration to enable/disable.
One comment I have on it tho, is there is no need for it to have a tick handler to count ticks.
It should be able to just pass in the world tick info from the render call like it does for partial ticks.

The only real issue is that we has no hard verification to justify the changes.

[asiekierka]

Besides, if we're hooking the cloud renderer anyway, why not let mods provide their own cloud renderers for their own worlds? That way, the optimization itself could be an addon which doesn't use ASM, and mods would benefit as well. I'm sure many would appreciate that, even mods which don't add dimensions like CloudMaster.

[Zaggy1024]

There is actually a hook for custom cloud renderers already, if I remember correctly. It's been a long time since I touched this code, though, so I can't remember why I decided against using that to hook this renderer.

[LexManos]

Yes there is already a sky and cloud renderer, And Asi you're just sitting here pissing me off. Your opinion is not wanted. Shut up. Stop spamming my issue tracker.
I understand what you are saying, I know why you are are saying it. And I don't know how much more I can make this clear I DONT FUCKING CARE ABOUT YOUR OPINION.
Forge HAS MANY performance enhancements to the vanilla code base. We are not JUST a compatibility layer, we are a major proving platform for new technologies.

No need for any more discussion beyond what has been requested of @Zaggy1024, if he can create a test harness that actually validates his claims of performance improvements then we can move on from there.