Wrong ref to Game Settings/Information after Game Load. about lateralgm HOT 8 CLOSED

Partially fixed in #70

To fix this I made the getGameInformation() and getGameSettings() method in the main LGM class create the mdi frame each time and have them dispose on close, this ensured the referenced stayed updated. Now this would not be an issue if the frames had a method to revert their controls to the properties of a new resource they could be instantiated once. But even my method here could be made better for the time being, needs to be discussed more when IsmAvatar joins the IRC today.

*** Update ***
They do have a revert resource method, but that method does not reset all the controls, this was causing some things to not be displayed correctly. My temporary solution creates a new frame each time it is oppened just like the other resources. A good enough fix for now, but the 3 frames need refactored and more thought before going forward.

from lateralgm.

I have further investigated this issue and come up with a much more graceful solution in the Latest LGM jar. I simply added...
LGM.currentFile.gameInfo = res;
to the commitChanges() method of GameInformationFrame and likewise for GameSettings, the issue only occurs with resources that are not instantiated.

from lateralgm.

Alrighty @IsmAvatar, I have done some very very thorough investigation of this issue, and it was not that hard to find it was right in front of my nose the entire time. Please pay attention closely as this bug needs to be addressed now rather than later, and also because this issue is breaking my fixes for #53

You start a new game, the game settings frame and all those main nodes in the top level of the tree, their frames are passed the resource from the default ProjectFile which is added to resMap.
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/file/ProjectFile.java#L273
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/file/ProjectFile.java#L338
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/main/LGM.java#L1094

They create a PropertyLinkFactory and set all the controls based on its property values.
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/subframes/GameSettingFrame.java#L681

Now you either hit new project or load an existing project and a couple of things go awry. First, a new ProjectFile is created and a new Game Info/Settings that is placed in the new resMap. Then in reloadPerformed we have the LGM passing for instance the Game Settings Frame the new Game Settings resource as the resOriginal, the frame then copies that into the main res and reverts some of the controls but not those linked to the PLF.
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/main/LGM.java#L727
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/subframes/GameSettingFrame.java#L962
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/subframes/GameSettingFrame.java#L929

The first problem here is that the Game Settings Frame was initialized with res being the resource from the ProjectFile, now we are doing it backwards and as a result we have mismatched references.

There are several courses of action.

1. We can just replace the ProjectFile's reference with the one our Game Settings Frame has, but this means this reference will never once change for the entirety of the session and all projects, you will essentially always be editing the same one unbeknownst to the user. The following is an example that does fix this problem, though this solution is not really as graceful and won't be needed anyway when we make multiple configurations and everything.

gameInfo.resOriginal = LGM.currentFile.gameInfo;
gameInfo.revertResource();
gameInfo.setVisible(false);
LGM.currentFile.gameInfo = gameInfo.res;
LGM.currentFile.resMap.put(GameInformation.class,new SingletonResourceHolder(gameInfo.res));

2. We could change the reload method to pass the new Game Settings/Information as res and not resOriginal. This requires that we recode the revert resource method to work like the default ResourceFrame should and also to recode PropertyLinkFactory to let you change the map and have it update its components, which also requires that all PLF's store a collection of what links they've made instead of simply returning them.
   https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/PropertyLinkFactory.java

gameInfo.res = LGM.currentFile.gameInfo;
gameInfo.revertResource();
gameInfo.setVisible(false);

@Override
public void revertResource()
{
  resOriginal.properties.putAll(res.properties);
  plf.ChangeMap(res.properties);
  setComponents(res);
}

3. We could just do away with the Game Settings/Info frames being static and recreate them every time you edit the resource and let them take the default behavior of every other ResourceFrame, this is the easiest solution but there are performance concerns I suppose because you had them even in their own thread before. Really not that big of a deal though because the second option still goes back through all the controls anyway, and since it would require a lot of pain in the ass coding, and also because you have to iterate other maps it could in fact be slower than this solution.

To be honest I find all of the solutions quite nasty and I am not really sure what you would prefer to have me do, or what you would have done in a situation like this, but I want to do this the correct way so the issue does not come back again and thus why I am seeking your guidance.

from lateralgm.

Good work, detective. Pretty much the same crap that I kept running into.

I'll go ahead and rule out option 1, since that is counter-intuitive for both the internal logic and the user.

Option 2 is the logical solution, both internally and for the user. However, I do recall running into issues with trying to do this, forcing me to have to intentionally do GS/Info's update/revert backwards from everything else, and I hated having to do it. I don't recall precisely what the issue is, but it may have gone away with some of the refactors afterwards, or you might have more luck/insight dealing with it as you have done in the past with other things I ran into issues with. Whatever the case, if you believe you can do this without breaking anything, I believe this is the preferable option.

Option 3 may possibly be viable at this point in time, however I am very hesitant. First I want to talk about the performance concerns. Creating widgets has always been a surprisingly huge overhead, incomparable to changing every single one of their values (which is actually quite negligible). I know this from experience with both methods. As for the threading, part of that was performance (trying to shrink the loading screen time), and part of it was to avoid a graphical glitch somewhere (either the tree or the game settings) that occurred sometimes from the render getting called before everything was available, so threading delayed the call and also gave us better control over when it was called.

As far as option 3, if you do believe it's completely viable despite what I've said, you are free to attempt it. Please do test thoroughly, repeatedly, on slower boxes, and with a stopwatch. Performance decreases are acceptable to a point.

from lateralgm.

@IsmAvatar Well before I make a decision let me discuss some changes that the second option would entail.

First both of the fields of the PropertyLinkFactory will need to remove the final modifier, the listener really don't but it should be able to anyway since you can usually change listeners for about everything else in Swing/Java.
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/PropertyLinkFactory.java#L29

Next, instead of just returning the new link we would need to also push it back into a collection and save all links made by this factory so that we can update them when we change the map of the PLF.
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/PropertyLinkFactory.java#L51

And finally, would it be better to have these changes made to a new class perhaps on top of PropertyLinkFactory, such as PropertyLinkFactoryManaged? That way the behavior of both is retained, you can still get a lightweight PLF if you don't plan on changing the map, or you can get a more managed one if you do.

Just to additionally touch base on option 3, I do know what you mean though LGM loads in less than 5 seconds for me, it takes me about 30 seconds to load some .NET applications such as Paint.NET so this is a valid concern. Currently the slowest editor to load is actually the script frame with JoshEdit, I believe because of the long list of keywords. But since we've removed the threading Game Settings/Info/Constants and Extensions frames are all created in the splash screen.

from lateralgm.

@IsmAvatar As of 8eb131f I have temporarily patched the issue with solution number one, I know it's not best but I just wanted to get a temporary solution in the main repo with my other changes.

from lateralgm.

@IsmAvatar The last patch actually turned out to not work, heh. But fear not as I went and made the changes necessary for the second solution, and I can report to you in the most ecstatic manor that checking for changes now works flawlessly and the duplicate ids issue has finally been fully resolved.

These were the two commits both to LGM and the plugin.
enigma-dev/lgmplugin@3d80bfa and f7e9c8c

The fix basically involved making property links implement a listener interface for when the factory that created them decides to change its map, at which point they update and revert their associated control. I additionally cleaned up issues in a commit slightly before that with the constants frame that existed before I got here as well as a line I removed that broke it worse. So all resources, of all types, and of all origins now work perfectly with recognizing changes and the entirety of the whole thing works! You can change a resource of any kind, instantiable or non-instantiable, leave the frame open and run the game and see your changes immediately and still close out and revert those changes if you don't want them!

All of this considered, if you are happy with the fixes, go ahead and close this ticket.

from lateralgm.

Can confirm that, after fumbling with learning how to save/load games, the original issue is resolved. I'll trust your regression tests were comprehensive enough.

I'm glad to hear that you were able to conquer it, and very quickly. I hope the code feels even more modular and logical now that all resources follow the same code path.

Looks like the Save/Load interface could use a little TLC next :-)

from lateralgm.