Decluttering the WorldRendererLwjgl class

[manu3d]

As I'm going through the class WorldRendererLwjgl, at this stage mostly to understand what is doing, I'm noticing a few things that perhaps might be useful to change even though the functionality wouldn't change at all. I.e. there are currently a bunch of methods called each frame with lines containing code such as:

• CoreRegistry.get(Config.class)
• CoreRegistry.get(Config.class).getRendering()
• CoreRegistry.get(Config.class).getRendering().getDebug()

My guess is that these can be replaced by initialize-once private fields. Unless any of the resulting objects changes at runtime. Can anybody confirm that these Config object are stable throughout one run of Terasology and can be fetched only once?

Also, there are a number of instances of code in this form:

PerformanceMonitor.startActivity("Something");
doSomething();
PerformanceMonitor.endActivity();​

resulting in some key methods, i.e. WorldRendererLwjgl.render() making for a very fragmented reading instead of, say:

doThis()
doThat()
doAlsoThis()
etc()​

I was wondering then if there's a good reason for keeping those bracketing PerformanceMonitor statements there or if I could tuck them inside those method instead.

 

[Cervator]

Sounds fine to me, but cannot really confirm from lack of expertise. Maybe @Immortius or @Florian or somebody to be sure

 

[Florian]

Can anybody confirm that these Config object are stable throughout one run of Terasology and can be fetched only once?

I think I said this before: No one currently active knows the graphic code inside out.

If you asking yourself "Can I rely on the code to have a behavior X?" Then you should also ask yourself "Is there any gurantee that it stays that way?" If you need then the behavior X, then the next question should be: "What can I do to ensure that it stays that way?" Good code makes it very hard to accidentally introduce bugs.

In the case of the config it can be that someone in the future exchanges a whole config object. this could be prevented by making some setters private and by the addition of a javadoc comment. I think however this is to much complexity an work for that little gain for now. So I think we should life for now with the fact that we can't rely on config objects to always be attached to the current config. This means:

• The config settings that can be changed via the UI or commands, should propably be always taken from the current config to allow the user to see the changes immidatly without a restart.
• The config settings that are guranteed to be constant, can savly be stored in final fields.

I was wondering then if there's a good reason for keeping those bracketing PerformanceMonitor statements there or if I could tuck them inside those method instead.

I like the idea of moving them into the methods, as it simplifies the calling method.

 

[manu3d]

Hi Florian, and thank you for your help.

It should be noticed that the Config-related code question is more of a general programming question rather than graphics-related. I.e. I seem to remember having seen various uses of the idiom "CoreRegistry.get(Config.class)" at method level rather than (once) at class level in non graphic-code.

You make a good point however that the issue should be handled defensively. I.e. perhaps one day we might have "presets" which might be handled swapping config objects rather than copying settings from the preset to the config object.

I guess one way I can handle this is re-obtaining the config objects once per frame, at the beginning of the render() method in the renderer. It's not quite as elegant as checking config.isValid(), but it is better than having the same config objects reobtained multiple times per frame, each time a method deep in the main loop needs them.

Again, thank you Florian!