Sample direction that FCs are going towards [RFR] #3258

vampcat · 2018-02-11T22:24:12Z

A small test to see how well FCs replace our current settings.

GooeyHub · 2018-02-11T22:35:56Z

Hooray Jenkins reported success with all tests good!

eviltak · 2018-02-12T02:42:27Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

@@ -77,6 +76,11 @@
    public static final String DUMP_SHADERS = "DumpShaders";
    public static final String VOLUMETRIC_FOG = "VolumetricFog";

+    public static final SimpleUri WINDOW_POS_X_ = new SimpleUri("engine:windowPosX");
+    public static final SimpleUri WINDOW_POS_Y_ = new SimpleUri("engine:windowPosY");


I think both of these can be put together into a single Setting<Vector2i> for easier access. However, this change will break things, so it may be better off in a separate PR.

Edit: This change already breaks things, so the change can be made in this PR if decided upon.

I actually favour this change, for reasons other than easier access. Rather than subscribe to either width and height, you can simply subscribe to one property. So I think let's make it a Vector2i in this PR, unless someone has a problem. This PR will be kinda messy and break-all-the-things either way.

I think it would be at least interesting to see a Vector2i setting in action. Let's give it a try.

eviltak · 2018-02-12T02:51:53Z

I consider this PR a great insight into the current shortcomings of FlexibleConfig, and thank you @vampcat for this effort. A couple of points:

I find the usage of new SimpleUri everywhere quite unwieldy. I think we can get around this by creating overloads of the SettingImpl constructor and FlexibleConfig.get that take a string parameter, convert it to a SimpleUri, and pass it to the SimpleUri overload.
I assume that you suggest the wrapper class over each FlexibleConfig only because of storing the setting ids as constants in the wrapper class. WIth the ability to pass strings directly to the relevant methods in FlexibleConfig, I think the constant ID fields should not be needed any more. However, keeping the constant ID fields has its own merits, so this is debatable.

In case we do decide on keeping the IDs as fields, it might be worth taking a look at the initial SettingKey approach introduced in FlexibleConfig #2757. That way, we will be able to avoid (if deemed a problem) the setting value casts when the FlexibleConfig.get call is inlined, as in

Terasology/engine/src/main/java/org/terasology/engine/subsystem/lwjgl/LwjglDisplayDevice.java

Line 84 in 7162356

    
           Display.setLocation((Integer)config.getFlexibleConfig().get(WINDOW_POS_X_).getValue(), (Integer)config.getFlexibleConfig().get(WINDOW_POS_Y_).getValue());

vampcat

With some initial tests, I feel like making a static class with methods like getRenderingFc() might be a good idea, so as to avoid things like (Integer)context.get(FlexibleConfigManager.class).get("engine:rendering").get("engine:windowPosX").getValue(), but that goes against the spirit of the context, so we can just split the above line into two.
It also feels like a good idea to have a get(Class clazz) constructor, so that we can morph the above statement into renderingFc.get("engine:windowPosX", Integer.class) or something. Feedback would be appreciated on this.

vampcat · 2018-02-11T22:25:24Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

@@ -28,13 +31,9 @@

 /**
 */
+@SuppressWarnings("WeakerAccess")


Added because the public String <settingName> often gave a access can be private when in fact, since these settings should be available to everyone, it should be public.

vampcat · 2018-02-11T22:25:55Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

@@ -77,6 +76,11 @@
    public static final String DUMP_SHADERS = "DumpShaders";
    public static final String VOLUMETRIC_FOG = "VolumetricFog";

+    public static final SimpleUri WINDOW_POS_X_ = new SimpleUri("engine:windowPosX");


The _ at the end is temporary, and will be removed as soon as I migrate all settings over.

vampcat · 2018-02-12T07:38:09Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

@@ -77,6 +76,11 @@
    public static final String DUMP_SHADERS = "DumpShaders";
    public static final String VOLUMETRIC_FOG = "VolumetricFog";

+    public static final SimpleUri WINDOW_POS_X_ = new SimpleUri("engine:windowPosX");
+    public static final SimpleUri WINDOW_POS_Y_ = new SimpleUri("engine:windowPosY");


I actually favour this change, for reasons other than easier access. Rather than subscribe to either width and height, you can simply subscribe to one property. So I think let's make it a Vector2i in this PR, unless someone has a problem. This PR will be kinda messy and break-all-the-things either way.

vampcat · 2018-02-12T07:39:10Z

engine/src/main/java/org/terasology/engine/subsystem/lwjgl/LwjglDisplayDevice.java

@@ -78,7 +81,7 @@ public void setDisplayModeSetting(DisplayModeSetting displayModeSetting, boolean
            switch (displayModeSetting) {
                case FULLSCREEN:
                    Display.setDisplayMode(Display.getDesktopDisplayMode());
-                    Display.setLocation(config.getWindowPosX(), config.getWindowPosY());
+                    Display.setLocation((Integer)config.getFlexibleConfig().get(WINDOW_POS_X_).getValue(), (Integer)config.getFlexibleConfig().get(WINDOW_POS_Y_).getValue());


I definitely don't like this cast, but I can't see any way around it for now.

Why, what's wrong with the cast?

That been said, get config.getFlexibleConfig() once and call it renderingConfig. It will reduce the clutter a bit.

P.S. I am aware config is of RenderingConfig type and it might be a bit confusing. Eventually that variable will go away though, right? When we get the config from the FCManager directly? And in fact, why can't we do it already?

vampcat · 2018-02-12T07:49:54Z

Also, @eviltak I totally agree about the overloaded String counterparts, I was about to make that change either way :D

Regarding the wrapper (point 2) : Not just the constants, the casting as well. The difference between (Integer)context.get(FlexibleConfigManager.class).get("engine:rendering").get("engine:windowPosX").getValue() and context.get(RenderingConfig.class).getWindowPosX() is huge, imo. That being said, it does introduce some extra classes, so..

Not sure what you mean by the Key class, since I really don't want to look at every commit and figure out which one has the Key class (it's not present in the net changes).

eviltak · 2018-02-12T09:22:38Z

@vampcat So you mean a wrapper class with wrapper methods for each setting like it currently is? While I can see the aesthetic appeal, it kind of defeats the purpose of using a FlexibleConfig, IMHO.

As for the casts, I see no way of avoiding them unless:

The setting is first extracted to a local variable:

Setting<Vector2i> windowPosSetting = config.getFlexibleConfig().get(RenderingConfig.WINDOW_POS);
Display.setLocation(windowPosSetting.getValue().x, windowPosSetting.getValue().y);

The setting type is passed to the method either via the generic type parameter or via a .class argument:

config.getFlexibleConfig().get(WINDOW_POS_X_, Integer.class).getValue()
// OR
// Side note: I find the generic type parameter placement in Java quite odd
config.getFlexibleConfig().<Integer>get(WINDOW_POS_X_).getValue()

or

The key method is used. Keys were disproved for being over-engineered and unwieldy (an assessment I largely agree with) and are effective only when all setting keys are stored for easy retrieval (via a constant or a static field or similar). For the implementation, please take a look at this old FlexibleConfig implementation. If there is renewed interest in this approach, I shall elucidate the usage of keys.

vampcat · 2018-02-12T11:30:58Z

Yes, it kinda defeats the purpose of FCs. Which is why I've tried to avoid that approach in this PR. However, as you can see, it feels way too verbose. Hence this PR - to get more eyes to look at it :D

I kinda like the cast approach, but again, that makes the code too verbose.

emanuele3d

Some minor changes and request for clarifications.

emanuele3d · 2018-02-12T20:54:05Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

@@ -77,6 +76,11 @@
    public static final String DUMP_SHADERS = "DumpShaders";
    public static final String VOLUMETRIC_FOG = "VolumetricFog";

+    public static final SimpleUri WINDOW_POS_X_ = new SimpleUri("engine:windowPosX");
+    public static final SimpleUri WINDOW_POS_Y_ = new SimpleUri("engine:windowPosY");


I think it would be at least interesting to see a Vector2i setting in action. Let's give it a try.

emanuele3d · 2018-02-12T21:01:44Z

engine/src/main/java/org/terasology/config/RenderingConfig.java

+        this.flexibleConfig = flexibleConfig;
+
+        // flexibleConfig.add(new SettingImpl<>(new SimpleUri("engine:pixelFormat"), 24));
+        flexibleConfig.add(new SettingImpl<>(new SimpleUri("engine:windowPosX"), 0));


Would it be possible to wrap these lines into a private method so that we could write:

addSetting(simpleUri, defaultValue);

I'm guessing there is some generics voodoo that can be used for this circumstances, so that the method pass the default value to the fc.add method without knowing its type?

Yes, it will be possible to do that. The type shall be inferred from the default value. I proposed something similar in the last couple of comments on my PR. This change will also have the added benefit of not requiring the SettingImpl class being exposed to the API, satiating @oniatus's concerns. I also think that the add(Setting setting) method should entirely be removed to prevent setting publishers from putting in their own potentially malicious Setting implementations, but that may be something to discuss at another time.

@eviltak: what do you have in mind when you think about a malicious setting implementation?

Regarding the SettingImpl class, I'm not bought on the idea that it shouldn't be public.

emanuele3d · 2018-02-12T21:06:59Z

engine/src/main/java/org/terasology/engine/subsystem/common/ConfigurationSubsystem.java

@@ -63,7 +67,8 @@ public void preInitialise(Context rootContext) {
        flexibleConfigManager.addConfig(new SimpleUri("engine:rendering"), renderingFlexibleConfig);

        flexibleConfigManager.loadAllConfigs();
-        // Add settings to RenderingFC
+
+        config.getRendering().loadDefaultRenderingConfig(renderingFlexibleConfig);


I'm confused by this line. What does this do? Loads default values from the existing rendering config into the rendering FC?

emanuele3d · 2018-02-12T21:12:19Z

engine/src/main/java/org/terasology/engine/subsystem/lwjgl/LwjglDisplayDevice.java

@@ -78,7 +81,7 @@ public void setDisplayModeSetting(DisplayModeSetting displayModeSetting, boolean
            switch (displayModeSetting) {
                case FULLSCREEN:
                    Display.setDisplayMode(Display.getDesktopDisplayMode());
-                    Display.setLocation(config.getWindowPosX(), config.getWindowPosY());
+                    Display.setLocation((Integer)config.getFlexibleConfig().get(WINDOW_POS_X_).getValue(), (Integer)config.getFlexibleConfig().get(WINDOW_POS_Y_).getValue());


Why, what's wrong with the cast?

That been said, get config.getFlexibleConfig() once and call it renderingConfig. It will reduce the clutter a bit.

P.S. I am aware config is of RenderingConfig type and it might be a bit confusing. Eventually that variable will go away though, right? When we get the config from the FCManager directly? And in fact, why can't we do it already?

emanuele3d · 2018-02-12T21:18:13Z

engine/src/main/java/org/terasology/engine/subsystem/lwjgl/LwjglDisplayDevice.java

@@ -94,7 +97,7 @@ public void setDisplayModeSetting(DisplayModeSetting displayModeSetting, boolean
                case WINDOWED:
                    System.setProperty("org.lwjgl.opengl.Window.undecorated", "false");
                    Display.setDisplayMode(config.getDisplayMode());
-                    Display.setLocation(config.getWindowPosX(), config.getWindowPosY());
+                    Display.setLocation((Integer)config.getFlexibleConfig().get(WINDOW_POS_X_).getValue(), (Integer)config.getFlexibleConfig().get(WINDOW_POS_Y_).getValue());


Same as above, about getting the renderingConfig once.

emanuele3d · 2018-02-12T21:18:50Z

engine/src/main/java/org/terasology/engine/subsystem/lwjgl/LwjglGraphics.java

-            config.setWindowPosX(Display.getX());
-            config.setWindowPosY(Display.getY());
+            config.getFlexibleConfig().get(WINDOW_POS_X_).setValue(Display.getX());
+            config.getFlexibleConfig().get(WINDOW_POS_Y_).setValue(Display.getY());


Same as above.

emanuele3d · 2018-02-24T19:08:45Z

@vampcat, @eviltak: the three renderer-related PRs have just been merged and we can focus once more on this one. Please let's get back on this and let's see what we can do about it.

eviltak · 2018-02-25T06:34:15Z

Readdressing something I briefly mentioned in a review comment (should address your clarification requests @emanuele3d):

I believe that we should change the current FlexibleConfig interface (and potentially the FlexibleConfigManager API too) to not allow setting/FC publishes to add their own instances of Setting/FlexibleConfig, for the reasons that follow.

I do not see the need to include more than one implementation of Setting in the future. Apart from SettingImpl, the only other implementation we have is MockSetting which I believe can also be removed. As such, I do not see the need to store in a FC an instance of a Setting implementation other than SettingImpl. Besides, even if we do add a couple of implementations of Setting, we can easily create overloads of the add method (or new methods entirely) that add a different Setting implementation.
Modules will not be allowed to add instances of their own Setting implementations to a FC. An external consumer may pass in a malicious Setting implementation like MockSetting that simply returns null in getValue and getDefaultValue. For this reason, I think it is better to prevent the setting publisher from creating a Setting instance and passing it to the FC - instead, the FC itself should be responsible for the creation of the SettingImpl object.
External consumers will not have to deal with any implementations of Setting - all their work can be done through the interface. This is in line with the usage patterns of other Interface -- InterfaceImpl pairs in the code base (Chunk -- ChunkImpl, BlockManager -- BlockManagerImpl, et cetera).

In code, boolean FlexibleConfig.add(Setting setting) should be replaced with Setting<V> FlexibleConfig.add<V>(SimpleUri id, V defaultValue) and Setting<V> FlexibleConfig.add<V>(SimpleUri id, V defaultValue, SettingValueValidator<V> valueValidator). Both of these methods internally create a SettingImpl<V>, add it to the settings map with the already existing rules, and return the created Setting. This ensures that the setting publisher has nothing to do with the instantiation of the setting object (and, in turn, does not have to deal with SettingImpl or a SettingFactory).

A similar approach for similar reasons can be taken towards the FlexibleConfigManager.addConfig method, which also requires that the config publisher create a new FlexibleConfigImpl instance and pass it to the method. Since no other FlexibleConfig implementation will ever be used, it is better to change the signature of addConfig to FlexibleConfig addConfig(SimpleUri id) and let the method create the FlexibleConfigImpl instance.

@vampcat @oniatus @emanuele3d please put forward your thoughts and suggestions.

emanuele3d · 2018-02-25T13:20:52Z

@eviltak, you need to look at your own communication and reasoning style as this really doesn't work.

Let me clarify:

I believe that we should change the current FlexibleConfig interface (and potentially the FlexibleConfigManager API too) to not allow setting/FC publishes to add their own instances of Setting/FlexibleConfig, for the reasons that follow.

This ignores and is in direct contradiction of my intention to get some experience first and then reassess. So, generally speaking it's a no.

I do not see the need to include more than one implementation of Setting in the future.

This is your personal judgement and I disagree. So, it's another no.

Besides, even if we do add a couple of implementations of Setting, we can easily create overloads of the add

This is another no, as we don't want to change the engine source to allow for additional setting types.

Modules will not be allowed to add instances of their own Setting implementations to a FC.

This imperious tone is absolutely out of place: you do not get to decide, at most suggest. Fourth no. Furthermore, I was curious about what you intended with malicious code and the example you provide doesn't sound malicious at all, just bad programming. If a module behaves like that it will be easily forgotten, never to be loaded again.

Summary: lots of circular reasoning, poor arguments and unacceptable communication style.

We work with what we have, we get some experience with it and we reassess later. Perhaps there will be scope for some of the changes you suggest @eviltak, but for the time being I don't want to discuss them again as it's a waste of time.

emanuele3d · 2018-02-25T13:25:43Z

@vampcat, please address my comments and let's see where we end up.

Also, we should keep in mind is that what looks verbose now, in a single FC, with lots of repetition, will eventually be decentralized into many parts of the code, i.e. individual nodes adding or obtaining their own settings.

So, we are going through a lot of cut&pasting right now, but eventually, a node developer or a module developers will only add or obtain the setting he or she is interested in: the whole list will be visible only UI side, and not necessarily all at once.

eviltak · 2018-02-26T07:07:46Z

@emanuele3d while I am otherwise ok with your judgement of my comment, you may have mistook the statement you called imperious. In no way am I domineering or making decisions for others through that statement -- it is simply a direct consequence of my proposal.

eviltak · 2019-10-16T08:36:36Z

Closing this PR as FCs have been replaced with AutoConfigs in #3723.

Sample direction that FCs are going towards

7162356

eviltak reviewed Feb 12, 2018

View reviewed changes

Cervator requested a review from emanuele3d February 12, 2018 07:19

vampcat commented Feb 12, 2018

View reviewed changes

emanuele3d suggested changes Feb 12, 2018

View reviewed changes

vampcat self-assigned this Feb 13, 2018

vampcat added this to the v2.0.0 milestone Feb 13, 2018

emanuele3d added the Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. label Feb 14, 2018

mkienenb added the WORK IN PROGRESS label Apr 28, 2018

mkienenb changed the title ~~Sample direction that FCs are going towards [WiP] [RFR]~~ Sample direction that FCs are going towards [RFR] Apr 28, 2018

eviltak mentioned this pull request Aug 13, 2019

AutoConfig #3723

Merged

eviltak closed this Oct 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sample direction that FCs are going towards [RFR] #3258

Sample direction that FCs are going towards [RFR] #3258

vampcat commented Feb 11, 2018 •

edited

Loading

GooeyHub commented Feb 11, 2018

eviltak Feb 12, 2018 •

edited

Loading

vampcat Feb 12, 2018

emanuele3d Feb 12, 2018

eviltak commented Feb 12, 2018 •

edited

Loading

vampcat left a comment

vampcat Feb 11, 2018

vampcat Feb 11, 2018

vampcat Feb 12, 2018

vampcat Feb 12, 2018

emanuele3d Feb 12, 2018

vampcat commented Feb 12, 2018

eviltak commented Feb 12, 2018 •

edited

Loading

vampcat commented Feb 12, 2018 •

edited

Loading

emanuele3d left a comment

emanuele3d Feb 12, 2018

emanuele3d Feb 12, 2018

eviltak Feb 13, 2018 •

edited

Loading

emanuele3d Feb 13, 2018

emanuele3d Feb 12, 2018

emanuele3d Feb 12, 2018

emanuele3d Feb 12, 2018

emanuele3d Feb 12, 2018

emanuele3d commented Feb 24, 2018

eviltak commented Feb 25, 2018 •

edited

Loading

emanuele3d commented Feb 25, 2018

emanuele3d commented Feb 25, 2018

eviltak commented Feb 26, 2018

eviltak commented Oct 16, 2019

Sample direction that FCs are going towards [RFR] #3258

Sample direction that FCs are going towards [RFR] #3258

Conversation

vampcat commented Feb 11, 2018 • edited Loading

GooeyHub commented Feb 11, 2018

eviltak Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eviltak commented Feb 12, 2018 • edited Loading

vampcat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vampcat commented Feb 12, 2018

eviltak commented Feb 12, 2018 • edited Loading

vampcat commented Feb 12, 2018 • edited Loading

emanuele3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eviltak Feb 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuele3d commented Feb 24, 2018

eviltak commented Feb 25, 2018 • edited Loading

emanuele3d commented Feb 25, 2018

emanuele3d commented Feb 25, 2018

eviltak commented Feb 26, 2018

eviltak commented Oct 16, 2019

vampcat commented Feb 11, 2018 •

edited

Loading

eviltak Feb 12, 2018 •

edited

Loading

eviltak commented Feb 12, 2018 •

edited

Loading

eviltak commented Feb 12, 2018 •

edited

Loading

vampcat commented Feb 12, 2018 •

edited

Loading

eviltak Feb 13, 2018 •

edited

Loading

eviltak commented Feb 25, 2018 •

edited

Loading