Flip "opaque_water" setting to "translucent_liquids"

[Xeno333]

Closes Issue #11330
by fliping "opaque_water" setting to "translucent_liquids"

[appgurueu]

This looks like a proper implementation of the "first approach" outlined in that issue (renaming the setting, flipping its meaning).

The only problem I see with this is that it effectively "breaks" the old setting. Are we fine with that? (And if not, is there an acceptably easy way to add "backwards compatibility"?)

[Xeno333]

This looks like a proper implementation of the "first approach" outlined in that issue (renaming the setting, flipping its meaning).

The only problem I see with this is that it effectively "breaks" the old setting. Are we fine with that? (And if not, is there an acceptably easy way to add "backwards compatibility"?)

Yes I can add a check for the legacy if the new setting is not found.

[Xeno333]

I am activly working on adding support for legacy. If this is deemed unneeded please tell me.

[Xeno333]

I am testing it now. I think I have it ready.

[Xeno333]

It does not use the old setting if present and defaults to the being transparent, but if there is no new value set with the new setting. I can not find any simple way to use the old setting, and allow it to be changed to the new setting, perhaps I can reset the new setting with an inverted value of the old setting.

[Xeno333]

OK its working now.

P.S.
Sorry my commits are a bit messy, I haven't done many PRs before. I also forgot I could reopen a PR when i acededntly closed it, thats why there is an an extra related one...

[Xeno333]

Ok it does work, and seems to properly handel the legacy setting. There seems to be no conflict now.

[Xeno333]

@appgurueu I have added backwards support and have tested it. It now will respect the old setting until you update it as the new setting.

[appgurueu]

I'm afraid there is a misunderstanding; I did not ask you to make changes, I just wanted to clarify some questions. (This does not mean that I want you to undo the changes.)

I think "migrating" (or at least respecting the old setting) is probably a good solution, though care needs to be taken to implement, and possibly document, this properly, since this will set a precedent for "migrating" settings.

[Xeno333]

I'm afraid there is a misunderstanding; I did not ask you to make changes, I just wanted to clarify some questions. (This does not mean that I want you to undo the changes.)

I understand and understood. I agreed that compatability is needed, thats why i updated it originaly.

I think "migrating" (or at least respecting the old setting) is probably a good solution, though care needs to be taken to implement, and possibly document, this properly, since this will set a precedent for "migrating" settings.

I see. If there is anyplace for me to write documentaion, do tell. I can in the comments but thats always a bit obscure when its needed later.

[Xeno333]

I am for using,
if (g_settings->exists("opaque_water")) g_settings->setDefault("translucent_water", g_settings->getBool("opaque_water") ? "false" : "true"); translucent_water = g_settings->getBool("translucent_water");
Insted of my oringal block. It is much clearer, as I couldnt find a setDefault at the time I wrote it + the fact I forgot that ? "false" : "true" works in C++, as I tend to use plain C where you can't directly compare strings (Objects)

[grorp]

All liquids are affected by this setting, not just water. Not sure whether this is relevant in practice (is there a translucent liquid other than water in any game for Minetest?), but if we're renaming and migrating the setting anyway, it would make sense to take that into account.

[Xeno333]

All liquids are affected by this setting, not just water. Not sure whether this is relevant in practice (is there a translucent liquid other than water in any game for Minetest?), but if we're renaming and migrating the setting anyway, it would make sense to take that into account.

Would you like me to make it "translucent_liquid" ?

[Xeno333]

Just updated it to translucent_liquids from translucent_water for clearity

[sfan5]

We don't use the .hpp file extension and code should not be in a header file.

[Xeno333]

This is what Lars(appguru) came up with, as its not a standard header file. Its included that way because its only run one time. I advocated for doing it in a .cpp file and then having a header file but Lars thought it was to much for one function that will only be called once. This is why we chose .hpp as to refrence it as a code header not a cpp file or regular header

[sfan5]

I'll leave it to the other reviews to decide then.

[grorp]

This is why we chose .hpp as to refrence it as a code header not a cpp file or regular header

doesn't make much sense to me, just didn't notice the different file extension

[Xeno333]

I can change it to .h but again its code too, i can just use .cpp and a sepreate .h but its only ever run one time.

This is why we chose .hpp as to refrence it as a code header not a cpp file or regular header

doesn't make much sense to me, just didn't notice the different file extension

I can change to .h if you think its better

[Xeno333]

I renamed to .h insted of .hpp

[appgurueu]

We don't use the .hpp file extension and code should not be in a header file.

This header is included exactly once, specifically in main.cpp, analogeous to defaultsettings.h, for a single function. There is no point in including it anywhere else. There will most certainly never be a point in including it in two different files.

defaultsettings.cpp has a corresponding header file, defaultsettings.h, which is also out of date. I do not see any benefit in the existence of defaultsettings.h.

It's just a function that's kept in a different file to not clutter main.cpp. As such, .cpp would be most appropriate, but including .cpp files is murky territory.

.hpp is the convention I use for "header-only" implementations such as this one. In our own sources, there is a weak precedent of this convention with catch.hpp. This convention seems to be relatively popular among "header-only" C++ libraries more generally (for example json.hpp).

I don't care about the file extension. But I do think that for this specific file there's no point in splitting it up in .h and .cpp.

[Xeno333]

We don't use the .hpp file extension and code should not be in a header file.

This header is included exactly once, specifically in main.cpp, analogeous to defaultsettings.h, for a single function. There is no point in including it anywhere else. There will most certainly never be a point in including it in two different files.

defaultsettings.cpp has a corresponding header file, defaultsettings.h, which is also out of date. I do not see any benefit in the existence of defaultsettings.h.

It's just a function that's kept in a different file to not clutter main.cpp. As such, .cpp would be most appropriate, but including .cpp files is murky territory.

.hpp is the convention I use for "header-only" implementations such as this one. In our own sources, there is a weak precedent of this convention with catch.hpp. This convention seems to be relatively popular among "header-only" C++ libraries more generally (for example json.hpp).

I don't care about the file extension. But I do think that for this specific file there's no point in splitting it up in .h and .cpp.

I agree, there is no point in splitting it when its only called by main. So I changed the file name to .h as the others do not seem to think using .hpp is good.

[appgurueu]

Seems fine to me. We have some header-only .h files already, though those are mostly using templates, hence have no other choice.

I'll use .h for header-only files in Minetest in the future then.

[Xeno333]

for some reason the checks faild but i have like 100kbps internet rn so i cant check it

[appgurueu]

for some reason the checks faild but i have like 100kbps internet rn so i cant check it

That's my fault. I have a fix, but I will wait for it to pass CI before pushing: master...appgurueu:minetest:master.

[Xeno333]

for some reason the checks faild but i have like 100kbps internet rn so i cant check it

That's my fault. I have a fix, but I will wait for it to pass CI before pushing: master...appgurueu:minetest:master.

oh ok, its the reason my checks are faling?

[appgurueu]

oh ok, its the reason my checks are faling?

Yes. I have restarted the failing check. It should pass now that I have pushed the commit. Apologies for the inconvenience.

[Xeno333]

oh ok, its the reason my checks are faling?

Yes. I have restarted the failing check. It should pass now that I have pushed the commit. Apologies for the inconvenience.

no problem, but I think it faild again...

[Xeno333]

oh ok, its the reason my checks are faling?

Yes. I have restarted the failing check. It should pass now that I have pushed the commit. Apologies for the inconvenience.

ye it faild again... imma check logs my internet is a little faster now, looks like its timing out