Detect/prevent GUI modification/deletion from other mods.

[eradicator]

Manipulating the *inside* of other mods guis is ofc broken.

TL;DR: The easiest method to deal with it is to properly check existance and .valid before you try to use a shared resource (gui, rendering, inventory, etc).

If you trully consider the GUIs as a shared resource, then it heavily contradicts with your 1st statement (which is pretty much the definition of an exclusive resource).

No. A shared resource can be arbitarily altered without my knowledge or consent. I am forced to share it with people i don't even know. That's exactly what GUI is. An exclusive resource is controlled soley by me and i am guaranteed that nobody messes around with it. "Good practice" is an afterthought that you can not rely on.

The killed GUI might be a vital choice in a scripted scenario (respawn menu, surface-switching logic) which are only displayed for specific events, and don't have any button to make them reappear.

That might be true. But what if my gui is *also* really important? What if the player dies during the scenarios surface switching menu and i need to display a respawn window now? I think i'll just switch everything to gui.screen.

Or some other mod might have also killed your menu button, because apparently killing other's GUI is fine.

Please stop putting words in my mouth. I am talking soley about gui.center. Clearing anything else is broken.

Moreover "valid" doesn't adress the issue in my 1st post. It doesn't prevent GUI modification, and doesn't detect them either (unless you run it every N tick, but it's ugly and still doesn't tell you which mod caused the mess).

Changing the *content* of a gui is broken and might even be malicious. No argument there. If somebody were to resize your window there wouldn't even *be* a way to detect that.

Oh but btw, when you create a new element in gui.center, do you actually check that gui.center is empty? Because if you blindly create new stuff there you might leave *both* mods in a broken state when elements are getting pushed off the screen. And that problem is even more fun when idiots start putting *invisible* stuff in gui.center where you have no idea when it becomes visible again.

Thinking about it again gui.center is probably a relic of the past and gui.screen should be used instead because there's no fighting for screen positions there, worst that happens is that stuff overlaps.

• Define a precise list of actions that every mod should support

Ye know, that would solve soo many problems. It would even make it possible to reorder top/left buttons by calling delete/create on them. But it's not really realistic, the community hasn't even managed to decided on a common modding framework. Everybody just does their own stuff. And it would only help if any other interaction with other mods guis would simultaenously be blocked - and that's basically a rewrite of the gui system. Aaand if the devs wanted to do that why did they design it as a shared resource in the first place.

For example, if you closed one of my mods' GUIs it would cause all sorts of crashes because I expect the GUI to be there.

I don't know how your mods work. If you properly handle player.opened you're likely not affected at all. If you don't then your mod is likely broken anyway.

Also the topic isn't really about you - or me. It's about @OP wanting to not have his mod crash. And the only way to get that is to properly check existance of the root gui element. There's currently 12 on_gui_* events, so that's at maximum 12 calls to a one-line check function. Probably much less because if your gui doesn't exist anymore you don't get events for it anyway (unless your event.element check is utterly broken too). So, it's either this or running around crying that other mods are evil and spamming incompatibilities and medieval public shaming and dealing with users complaining when yet another mod breaks yours. It's an uphill battle you can't win.

Maybe you've only dealt with GUI modding so far, but this shit happens everyday with entities. There's tons of mods that don't properly raise destroy/revive and it's easier to make your own mod more resiliant than to try and track down and convince everybody of "the right way". And devs are ignoring it because changing the defaults to destroy{raise=true} etc "could break some mods".

[Creidhne]

No. A shared resource can be arbitarily altered without my knowledge or consent. I am forced to share it with people i don't even know. That's exactly what GUI is

I agree Factorio indeed expose mod GUIs as a shared resource. I think we'll also agree that in practice, no mod really support arbitrary modification of their GUI, and assume nobody will mess with theme to some degree. So I think the only disagrement here is about definitions of shared/exclusive (whether it's defined by API, or usage).

My point of view is totally different about entity/inventory or similar. A mod creating any entity should expect that the base game or any script might modify/delete this entity. I'm pretty sure no mod with a GUI supports the modification/deletion of "any" of its GUI elements (and in my opinion, it shouldn't have to).

But what if my gui is *also* really important? What if the player dies during the scenarios surface switching menu and i need to display a respawn window now?

I would just leave the other GUI in peace and share the screen with it. Sure it brings the problem of pushing some GUI out of the screen. But I find that much more preferable than killing GUIs randomly.

Please stop putting words in my mouth. I am talking soley about gui.center. Clearing anything else is broken.

Putting any word in your mouth wasn't my intention. I didn't intend to mean that it's something you said or think.

I was just stating that if the API allow to mess with other GUIs, and some modders does it without too much restrictions (I didn't mean to include you in the list of modders without restriction), then it's pretty much impossible to make any working GUI.

do you actually check that gui.center is empty?

Well no I don't. As I said I don't mind sharing the screen with other mods.

And that problem is even more fun when idiots start putting *invisible* stuff in gui.center where you have no idea when it becomes visible again.

Well thanks to you I learnt that I was an idiot today. If my "apparently killing other's GUI is fine" offended you in anyway, I consider us even now .

Why would I bother destroying/creating my GUIs, when hiding/showing it saves both dev time, and player's CPU time ? The only reason I would bother to rebuild my GUI from scratch ouside on_init/on_configuration_changed is if it gets deleted by other mods, which I can't even properly detect.

As far as I can see in this thread, I'm not the only modder that assumes that his GUIs are to be left untouched. Even the people destroying GUIs in center seem to agree that cross-mod GUI modification/deletion should be heavily restricted. I don't think there is any idiot in the story, there are just people with different needs that do incompatible things with a way too much permissive API.

Thinking about it again gui.center is probably a relic of the past and gui.screen

I will probably do that too. However all it takes is 1 modder to start deleting stuff there because he unilaterally decided that it was fine to do so, and my initial problem still stands. I have not seen such usage yet, but I simply don't want to dig through the code of 50+ mod lists to find the conflict.

And it would only help if any other interaction with other mods guis would simultaenously be blocked - and that's basically a rewrite of the gui system.

Well only the purple guys knows. My uneducated guess is that since all LuaGuiElement already have a property containing the name of the mod which created them, I would be surprised that adding options to prevent cross-mod deletion requires anything close to a full rewrite.

why did they design it as a shared resource in the first place.

I can't speak for the men in purple. However I can say I have seen valid/legitimate usage of cross-mod GUI modifications, like for in-game Wikis. So I'm not advocating for full removal of cross-GUI modifications. Just for a way for the mods who don't have any use of that feature to not have to dig through horrible bug reports when it happens.

[eradicator]

I think we'll also agree that in practice, no mod really support arbitrary modification of their GUI

I already agreed several times that modification is unacceptable. But i do not consider deletition to be a modification.

My point of view is totally different about entity/inventory or similar. A mod creating any entity should expect that the base game or any script might modify/delete this entity.

Not without notice. Contrary to a GUI which has exactly one possible position an entity can be anywhere. If someone deletes an entity without raising an event i can't cleanup any linked entities without either caching tons of data or scanning the entirety of every surface to find the broken entities. I'm not talking about simple stuff. I'm talking about when you have several hidden entities that are logically linked together in a way that only the creating mod can know.

I would just leave the other GUI in peace and share the screen with it. Sure it brings the problem of pushing some GUI out of the screen. But I find that much more preferable than killing GUIs randomly.

Creates a broken gui though and leaves the user to deal with it. Possibly not being able to close either of the guis because the close buttons are both off-screen.

Why would I bother destroying/creating my GUIs, when hiding/showing it saves both dev time, and player's CPU time ?

Last time i tested (with a 10k+ elements gui) hiding it had no meaningful saving effect. Because the gui still exists. Might be ok for small things with < 100 elements or stuff that is used very often, but not anything else. And i just don't see how it saves dev time to do a simple unhide_or_create().

The only reason I would bother to rebuild my GUI from scratch ouside on_init/on_configuration_changed is if it gets deleted by other mods, which I can't even properly detect.

Eh? Detecting existance of one single element is trivial.

However all it takes is 1 modder to start deleting stuff there because he unilaterally decided that it was fine to do so, and my initial problem still stands.

Which is exactly what i said above. Make your mod resiliant and stop caring. Maybe you convinced me. Maybe i'm moving to gui.screen. But nothing said in this thread will make gui.clear() go away. In the time you wasted on this thread you could've already fixed it.

since all LuaGuiElement already have a property containing the name of the mod which created them

Huh. Changelog says 0.17.59. Never noticed that. Will certainly be helpful.

[raiguard]

I'm not aware of any friendlier mechanism to tell other mods that they need to close their gui *right now*. My own stuff is ofc designed under the assumption that destroying the root is the normal way for others mods to close my gui. What other method do you propose? And please don't tell me i'm supposed to contact every single mod author to request a remote call interface...

Use player.opened. That will solve all of your problems without you having to screw with other mods' GUIs.

[eradicator]

I'm not aware of any friendlier mechanism to tell other mods that they need to close their gui *right now*. My own stuff is ofc designed under the assumption that destroying the root is the normal way for others mods to close my gui. What other method do you propose? And please don't tell me i'm supposed to contact every single mod author to request a remote call interface...

Use player.opened. That will solve all of your problems without you having to screw with other mods' GUIs.

Already doing that. And switching to gui.screen anyway. And you still don't get that "convincing" me does not change the problem. My old pseudocode:

Code: Select all

local function open(player)
  player.opened = nil
  player.gui.center.clear()
  player.opened = mygui()
  end

Tried with your recipe book which properly closes. But @OPs Dana doesn't react to on_gui_closed. Which btw is documented as "This is only called if the player explicitly closed the GUI." Soo... still arguing against windmills.

[raiguard]

If a mod doesn't handle it properly, then it's that mod's fault. It's not your responsibility to "fix" them.

We had a lengthy debate about this in the #mod-making channel on the discord and pretty much everyone who participated (including one of the developers) agrees on that sentiment. You're trying to solve something that isn't your problem.

I think that's where the main difference in opinion is.

[eradicator]

You're trying to solve something that isn't your problem.

And you're trying to *not* fix something that *is* your problem. Namely the crashing of your mod after clear(). Caused by an unknown number of future modders who will never read this thread. Face reality or face all the future bug reports that *you* will get - your choice. Or you could try your luck at having "root elements can't be cleared" enforced in the engine so that people have to do for-destroy() loops :p.

[Creidhne]

But @OPs Dana doesn't react to on_gui_closed. Which btw is documented as "This is only called if the player explicitly closed the GUI." Soo... still arguing against windmills.

And that's something I'm willing to change if there is any need (if I get to understand what it does, because the API doc is extremely ... concise).

If some mod politely asks mine to get my GUIs hidden, I'll gladly comply (note that I said hidden, not destroyed).

Namely the crashing of your mod after clear().

The crash was absolutely not the problem I came on this forum for. I was perfectly aware of the existence of "valid" before posting, and would have never posted if it solved all my issues. The problem is that a stateful GUI gets closed (which I could make stateless & reconstructible if I put some dev time into it), that my handlers now have stale reference to GUI elements, and that my other GUIs & other pieces of code also have stale references, that my GUI (which is necessary for the player to go back to its surface/controller) is potentially gone leaving the player stuck into nothingness.

And my only way to detect that right as it happens is to "valid" the full GUI every tick (just checking "center" works only for people clearing center, not for general UI modification). Moreover making the full GUI stateless & reconstructible might be easy for mods with 1 frame & 2 buttons, but in my case I estimate it'll be too much work to just support interference from a few mods (which from what I've seen, would be equally buggy should a mod start messing with their GUI the same way as they did with mine).

[raiguard]

You're trying to solve something that isn't your problem.

And you're trying to *not* fix something that *is* your problem.

I already had to "fix" this in one of my mods because someone was clearing gui.screen. It's doable, but the point is that it shouldn't be needed. You're doing something that is unneeded, unhelpful, and unrecommended, and you're completely ignoring everyone trying to tell you of that fact.

You're interfering with other mods' operations purely because you want your GUI to be front-and-center. What if the other GUI is DESIGNED to exist alongside other windows? Then you've just broken that mod's functionality completely. You're intentionally BREAKING the mod and expecting them to have to fix it themselves.

I don't know if you are aware, but on_gui_closed also fires in the case that player.opened is replaced with something else. The line in the docs saying "This is only called if the player explicitly closed the GUI" is false, and needs to be removed.

[eradicator]

shouldn't be needed.

Tis tho. Sadly this universe isn't perfect. Tough luck. Idealism and Pragmatism don't go well together.

you're completely ignoring everyone trying to tell you of that fact.

Not sure if you're trolling now, or if you're just not reading what i write. I aready said i'm moving to gui.screen. So there's no need to center.clear() anymore, and i was always against clearing anything but center. Certainly not screen which was explicitly made to allow overlapping stuff coexisting. So. Calm down. And then maybe you finally understand that not handling clear() will just postpone the inevitable next crash.

on_gui_closed also fires in the case that player.opened is replaced with something else

your recipe book which properly closes [...] react to on_gui_closed

Yea. I happen to know what i already said. Maybe you should think about my "*old* pseudocode" again - which btw was never even on the portal. Clear() was only ever an afterthought.

Can i stop repeating myself now? I might've said sorry if you had been a little calmer.
______________________

And that's something I'm willing to change if there is any need (if I get to understand what it does, because the API doc is extremely ... concise).

Events don't "do" anything but you know that already ;). on_gui_closed is basically just a nice request "hey, please close your gui so i have some space to open another one". Well, or not open another one. The event is implicitly raised when player.opened is reassigned, even for player.opened=nil. But only if it was not nil already.

The problem is that a stateful GUI gets closed

I guess i was always subconciously avoiding statefulness because of all this chaos that can happen. That's a very valid point though.

I could make stateless & reconstructible

Imho simply not crashing would go a long way. Ok sure you lose the state if you just start from scratch, but it safes the user the trouble of losing progress (not everyone has 1-minute-autosave) and it gives them a chance to notice a pattern instead of being confronted with a wall of text. I.e. someone might notice much earlier that "oh, everytime i open modX your mod is reset afterwards". Because when stuff crashes most people are quick to uninstall. At the very least you should do a conrolled crash and preset a useful error.

After looking at your mod i'm not sure what huge amounts of state you're supposedly storing though. All i see is two items lists, two checkboxes and one textfield. Hm...and for the graph selection one box and a number again. (Oh, and i can make it crash with rendering.clear(), suprise! Yes, that can and has happed :p).

that my handlers now have stale reference to GUI elements

"Good practice" says that you need to .valid'ate every reference before every access if you store them in global. That's just how it is. That's what every mod dealing with entities does.

(just checking "center" works only for people clearing center, not for general UI modification)

Manipulation is realistically impossible to detect. Especially without stateless data. Just think of all the sytle properties text labels, etc. And even if it was it would probably be more expensive than to reconstruct the whole thing.

Clear() is easy to detect. You probably don't have more than a handful of elemetns that are directly owned by root. And for the player to be not left stranded in Dana it would be sufficient to check every 1~5 seconds. Ugly yes. Welcome to defensive programming :/.

Which begs the question: Did someone actually change anything deeper than gui.screen, gui.center, etc? Because if you're not using the same words as i am there can't be a meaningful discussion. And i strongly distinguish between clear() and manipuate/change.

[eradicator]

As a little bonus here's something that shows that invisible guis eat GPU for breakfast. Might need to up the numbers if you have a strong gpu.

Code: Select all

/c x = game.player.gui.center.add{type='flow',visible=false} for i=1,100000 do x.add{type='frame'} end

[raiguard]

I see, I assumed that since you were moving to gui.screen that you would continue clearing there. If you're not clearing it, then it's all good. But I still stand behind what I said about gui.center.

Sorry for getting worked up.

[Creidhne]

Oh, and i can make it crash with rendering.clear(), suprise!

I stand unsurprised. I was caught off-guard by the possibility of cross-mod GUI modification, but was fully aware of the potential rendering issues. I can think of dozens of way to make my mod bug/crash through commands or other mods, and I'm pretty sure I could find as many ways to put any mod out there with a GUI/rendering into a corrupted/inconsistent state.

For rendering issues, even checking/fixing every N tick is not realistic for performance reasons.

I'm not well versed into manipulating entities through API, but I'm pretty sure I could break quite a few mods too with an extremely liberal use of the API.

Manipulation is realistically impossible to detect.

And I fully agree. Realistically, Any modder could spend dozens of hours of dev time trying to make a mod fool-proof to "arbitrary" intrusive behaviour from other mods, and still not being 100% fool-proof (or destroying performances through extensive checking of the game state). On Dana's there has already been addition of some fool-proofness code, there will be more in the future. But dedicating significant dev time on that at the price of delaying much needed work on the core features is simply a mistake.

At some point a line has to be drawn between making a mod 100% cross-mod full-proof (a lost cause), and just accepting that conflicts will happen and you'll just have to investigate/fix efficiently bug reports as they come. The original purpose of this post, is to either find realistic ways to prevent these unwanted modifications from happening, or at least to quickly find the mod causing it. From what I've read so far you have proposed no solution for this problem, only workarounds for specific very cases.

Because if you're not using the same words as i am there can't be a meaningful discussion. And i strongly distinguish between clear() and manipuate/change.

I think my thread's title is pretty explicit on the matter. I've been talking from the start about any arbitrary cross-mod GUI modification/deletion. You're the one trying to restrict the topic to gui.center.clear().

[eradicator]

and I'm pretty sure I could find as many ways to put any mod out there with a GUI/rendering into a corrupted/inconsistent state.

Yea, never doubted that. It was more of a joke/annecdote. But, i'm kinda paranoid regarding crashes because - as mentioned above - they destroy user progress. For example my Animated Stockpiles mod is resistant (as far as i know) to rendering.clear(). Worst case it misses a few renders. Even has a command to fix them. Sadly is_valid() isn't nearly as fast as LuaEntity.valid - because it scales linearly with total render object count - so there's that. And ofc it's only a simple mod, but as it looks like you're building the whole graph at once anyway i wouldn't say it's impossible. Completely different topic though.

I think my thread's title is pretty explicit on the matter. I've been talking from the start about any arbitrary cross-mod GUI modification/deletion. You're the one trying to restrict the topic to gui.center.clear().

I wasn't trying to restrict the topic to anything. Sorry if i made that impression. I just think clear and modify are too different to be meaningfully discussed in the same thread. I consider arbitrary modification to be kind of a "natural desaster" which you can't do much about. But deletition of the whole gui root is imho a solvable problem because it imho only needs a valid check on a very limited set of elements.

The original purpose of this post, is to either find realistic ways to prevent these unwanted modifications from happening, or at least to quickly find the mod causing it. From what I've read so far you have proposed no solution for this problem, only workarounds for specific very cases.

That's because i do not know and can not think of any realistic general solution to do that. Or even of a realistic modding interface request to help. Forbidding clear() on root elements might raise awareness to the issue, but it won't prevent it. Maybe a "don't touch this" tag on every element? "Store more data" requests tend to be denied though. Not sure how get_mod() reacts to modifications, if it works like last_user on entities maybe there's some way. But even then you'd still have to recursively walk the whole gui structure.

I've been talking from the start about any arbitrary cross-mod GUI modification/deletion.

See, you're treating them the same again, which from my pov makes no sense. I know that clear() happens (*cough*), but as far as i can tell "arbitrary modification" is just a theoretical situation you've come up with. From my point of view you're refusing to talk about solutions for clear() because they don't also fix modify.

[Creidhne]

From my point of view you're refusing to talk about solutions for clear() because they don't also fix modify.

You can rest assured, my next major release will have no problem handling gui.center.clear(). I also plan to handle on_gui_closed.

I know that clear() happens (*cough*), but as far as i can tell "arbitrary modification" is just a theoretical situation you've come up with.

And someone running center.clear() was a theoretical situation until it happened. Apparently screen.clear() is not a theoretical situation anymore. Maybe another modder might just put "visible" to false instead of flat out destroying another mod's GUI (which is what I would have done personally, if I hadn't a "let the other mods' GUI alone" policy). Maybe some funny mod will just put my screen GUIs at x=10^8 as a way to hide them (I estimate this one to be extremely unlikely though).

Fool-proofing my rendering usage is the same issue. Just detecting/fixing a full clear is perfectly doable. If the other mod just deletes some of my rendered items, a fast & optimized solution is almost impossible because of the intrinsic performances of the rendering API. Now if they start messing with color,positions, or anything else... Well, I'm just done for.

So for me it makes a lot of sense to ask for a unique solution for what is a huge Pandora box of potential problems. The generic solution to the full family of problems could have been much simpler than fixing individual problems as they come. That I could not know before I asked.

Right now I have the feeling that I'm making a website and some user has found a way to do arbitrary injection of SQL code to add 1M of currency on their account. I'm asking if the framework has any feature to just prevent/detect any SQL injection, and the answer I get is that I should just scan constantly the DB for accounts with 1M+ currency and remove 1M currency. I don't consider that a valid solution to the problem: it even just feels like a waste of dev time.

[eradicator]

You can rest assured, my next major release will have no problem handling gui.center.clear(). I also plan to handle on_gui_closed.

/me rests assured.
Glad you got that fixed at least :).

As for the theoreticallity of shit hitting the fence... i've been here for quite a while, seen and heared things. Came to terms with the general situation. So i guess my view about what is and isn't likely to happen differs from yours. In case it wasn't clear above: I'm not even in favor of GUI being a shared resource at all. If it was up to me it'd go into each mods sandbox. But that sort of fundamental change seems unlikely to me to happen.

Btw if you want improvements to the framework this is the wrong subforum. For that you have to go to "modding interface requests". And that's not really a discission forum either, so you'd need at least a basic idea on how to fix it all.

If you're really comfortable with lua you could try sandboxing your own code so that you don't have to .valid on every single object but can still throw proper errors. But that's kinda frowned upon because so many people do it wrong and end up with even worse bugs.