Refacto: rewrite VisibleIf Feat: manage LogicalOperator #11
Conversation
|
damn, u have change things ont it for people not using schema ... will adjust with your modifications |
|
What i've done.
|
Mod Configuration Menu/Mods/BG3MCM/ScriptExtender/Lua/Client/IMGUILayer.lua
Outdated
Show resolved
Hide resolved
|
add isVisible operator. use case i need it: settingX (a intSlider) is visible if settingA > 0 or settingB > 0 or setttingC > 0 SettingZ is visible if settingX > 0 (but if it's visible) so it's a (A>0 or B>0 or C>0) and X>0 Other usage is to make 2 setting share same visiblity without rewrite all conditioning edit: ready to review, just tell if u want to move every visibility things to a dedicated file |
| if v and (logicalOperator == "or") then | ||
| visible = true | ||
| break | ||
| elseif (not v) and (logicalOperator == "and") then | ||
| visible = false | ||
| break | ||
| end |
There was a problem hiding this comment.
Why is this being evaluated in an if statement?
There was a problem hiding this comment.
because of the break to stop looping all condition and stop as soon as possible.
"and" have true initial value and turn false if one condition is false -> break
"or" have false initial value and turn true if one condition is true -> break
no need to test more conditions. so break needed . it's not a simple assigment
| operators = { | ||
| ["=="] = function(a, b) return a == b end, | ||
| ["!="] = function(a, b) return a ~= b end, | ||
| ["<="] = function(a, b) return a <= b end, | ||
| [">="] = function(a, b) return a >= b end, | ||
| ["<"] = function(a, b) return a < b end, | ||
| [">"] = function(a, b) return a > b end, | ||
| ["isVisible"] = function(a, b) return a == b end | ||
| }, |
There was a problem hiding this comment.
Something like this shouldn't be in the IMGUILayer class
There was a problem hiding this comment.
that's why i said it need a visibilityManager class. I move them here to avoid recreating them at each condition and use CPU for nothing
| [">"] = function(a, b) return a > b end, | ||
| ["isVisible"] = function(a, b) return a == b end | ||
| }, | ||
| uiElementByName = {} |
There was a problem hiding this comment.
I don't like the idea of attaching more lookup stuff to the class... we haven't been needing to do this
There was a problem hiding this comment.
i write exemple of the requierement of that. test value is a fact, but if widget or section is invisible, the widget value a no meaning. i use VisibleIf operator to avoid MCM to check visibility of parent. and make user responsible to check it depending of what he have make.
uiElementByName is the simpliest way to achieve retrieve it without tons of looping and CPU. doing the map cost nothing but few byte..
again, just tell to do visibilityManager class
|
Thank you for your effort on this PR. However, I feel that it adds too much complexity and opens up for technical debt (worsening maintenance) for an issue that has already been largely addressed. I have several mods already, so maintenance is very important to me. |
|
control over user input is far more maintainble as i write it. and it's done at one place and only once. Currently, it's validated each time, and in multiple method !!! chance of visibilityTrigger storage make it possible. |
|
rebase and move to other class. Noting to do with PR but: since my rebase, widget description are now next to le the (on le right). was under before: that was WAY better to read. even on my 34 UW, now it's nearly take all. |
| end | ||
|
|
||
|
|
||
| function IMGUIVisibilityManager:EvaluateVisibleIf(modGUID, visibleIf, elementName) |
There was a problem hiding this comment.
can be local function in manageVisibleIf
here is the new one.
refcto of management of isVisible and more