adventuregamestudio / ags

AGS editor and engine source code
Other
707 stars 159 forks source link

Deleting a GUI doesn't update Custom text-window setting #2542

Open Crystal-Shard opened 1 month ago

Crystal-Shard commented 1 month ago

Describe the bug Parts of the editor reference to GUIs by number.

AGS Version 3.6.0.53

To Reproduce Steps to reproduce the behavior:

  1. Create a game with three GUIs.
  2. In general settings, set Custom text-window GUI to 2
  3. Delete the first GUI.
  4. Note that Custom text-window setting is still 2, whereas it should be 1. This will crash the game when it displays a textbox.

Expected behavior If a GUI is deleted, references to GUIs should take that into account. I don't think this is a priority since it has an obvious workaround.

Desktop (please complete the following information):

ericoporto commented 1 week ago

CurrentGame has a NotifyClientsGUIAddedOrRemoved, which I guess could be used - was it removed or added? Can't tell. And it's in AGS Types, so it is had to change to add an extra parameter...

In general it's a bit hard when the GUI has no unique ID of sorts, if it uses a number as reference, then any modification to the sequence has to update the settings on a one-by-one change - you delete one gui and then you create another and the like. On each update, it has to sync this option in General Settings.

If this option used the GUI name, then yeah it could break if the GUI name changes, but then it's less things to track.

I can't tell where this code should go, I guess it would go to AGSEditor.cs.

ivan-mogilko commented 1 week ago

Above scenario is not the only one that may cause change of ID. There are three:

GUIs may not be only one kind of item that may be referenced from other properties. However, there are items that retain their IDs when something else gets deleted (sprites and views), and then items which are internally referenced using fixed indexes (audio clips). These two groups of items do not cause trouble when something else gets deleted prior to them, but they still cause other two scenarios (deleting the referenced item itself, and changing ID by command).

Thinking of other types that may be referenced from a property, I remember only Rooms (StartingRoom in character properties) and Characters (CharacterID in InventoryWindow properties).

I suppose it would be nice to deal with this consistently.


Another thing, not exactly required for this, but for convenience it may be worth having these properties have dropdown lists for selecting an item. Similar lists already exist, and there's a parent TypeConverter class that implements list-based selection, and have an actual list assigned from elsewhere (from inside AGSEditor).

ivan-mogilko commented 1 week ago

In regards to events like NotifyClientsGUIAddedOrRemoved and backwards compatibility, I did not check the code yet, but suppose that such situations are commonly solved by adding new more suitable event(s), and calling the old one just as before.

But of course, as much as possible, this "property update" process should be kept within the AGSEditor.