#4609 closed defect
GUI: GMM crashes when running in 320x200 and 320x240
| Reported by: | SF/sanguinehearts | Owned by: | lordhoto |
|---|---|---|---|
| Priority: | blocker | Component: | GUI |
| Version: | Keywords: | ||
| Cc: | Game: |
Description
When playing Maniac Mansion NES(US)ROM PRG Section on the iPhone RC1(downloaded from the cydia sources) the game plays fine, however when I load up the menu with F5 and click on 'Options the game crashes to console and displays
'ERROR: Widget <ScummConfig.Ok> has x + w >'
when clicking Help it also displays
'ERROR: Widget <ScummHelp.Close> has x + w'
This crash does not occur with MM C64 or ZAK C64 on the iPhone and does not occur with MM NES running under a trunk build on Windows.
I have attached screenshots of the console during the crash
Just my two pence but could this be caused by the difference in resolution for the NES version of MM?
Ticket imported from: #2859401. Ticket imported from: bugs/4609.
Attachments (4)
Change History (28)
by , 15 years ago
| Attachment: | IMG_0067[1].PNG added |
|---|
comment:1 by , 15 years ago
| Owner: | set to |
|---|
comment:3 by , 14 years ago
| Priority: | normal → blocker |
|---|
comment:4 by , 14 years ago
vinterstum, any comments?
sanguinehearts, when you tested MM NES on windows -- did you use any scalers or aspect ration correction? It would be interesting to hear what happens if you run it on windows (or linux, mac, whatever) with 1x scaler, once with and once without aspect ratio correction. (Background: The error could be caused by some ScummVM GUI items not being set up properly for the small screen size on the iPhone).
comment:5 by , 14 years ago
When trying MM NES on windows I didnt use any scalers, Though I have just tried it now and the same issue occurs but earlier in windows than it does on the iPhone. When running in 1x with or without aspect ratio correction pressing F5 actually causes the error instead of using the menu 'help' and 'options' the iphone console cut off some of the error before and I can provide the full thing now ERROR: Widget <ScummConfig> has x < 0 (-30)!
comment:6 by , 14 years ago
| Component: | Engine: SCUMM → --Unset-- |
|---|---|
| Game: | Maniac Mansion |
| Owner: | changed from to |
| Summary: | iPhone GUI: Crash in MM NES → GUI: GMM crashes when running in 320x200 and 320x240 |
comment:7 by , 14 years ago
Indeed, the crash is not specific to Maniac Mansion, nor to the iPhone: By setting the scaler to 1x, I can also trigger a crash in e.g. Monkey Island running on Mac OS X.
comment:8 by , 14 years ago
This seems to be a reflow issue... If I open the GMM via F5 while using a 2x scaler, and the reduce to 1x scaling *while it is is open*, then things work fine. Also, when I open the GMM in 2x mode, then close it, then change the scaler to 1x, then open the GMM it works fine, too.
So only if I *first* change to 1x mode, then open the GMM for the very first time, does it crash.
comment:9 by , 14 years ago
I assigned this to LordHoto, since the cause of the crash is revision 46947 , which added a call to reflowLayout() to the GuiObject constructor. Removing this call fixes the crash.
It might be possible to solve this particular crasher while retaining the desired side effect of the reflowLayout call by moving it to the Widget constructor instead.
However, I find this call highly suspicious regardless; calling a virtual method in the constructor of the base class like this will not invoke the overloaded version in C++, after all. As such, the code is surprising if one is not quite familiar with the idiosyncrasies of C++.
So instead of trying to call reflowLayout in some constructor, I think we should instead call it where it is necessary. If there are places where it is necessary besides when the screen res changes or a dialog opens, then it should be added there.
comment:10 by , 14 years ago
It is intentional to call the GuiObject::reflowLayout implementation from the constructor. Just check the commit message for that. Actually our GUI *should* create the layout information regardless whether the widgets are created or not, thus it should be the correct rect setup there. Guess that (i.e. my "assumption" about the layout manager) is not the case then, thus we will get an invalid size. I just can repeat it, this has nothing to do with calling a virtual method from the constructor.
Also reflowLayout should be called on every screen resolution change, at least in case our backend supports screen change events and all engines correctly call the gui handler for that. I guess most (all?) just don't do that, and I think KYRA is no exception here...
Actually that manual handling of those things is one of the reasons, why I introduced all the event listener code, so the GUI could just setup itself to catch all events, even though it's not active and handle it transparently to the user. Of course I never came around to do that...
comment:11 by , 14 years ago
I know that the crash is not coming from the fact that a virtual method is called in the constructor -- I merely wanted to point out that besides the issue at hand (a severe crash), I also consider it very bad style to cal a virtual method in the constructor. And this case also seems to be unjustified.
I don't understand the second paragraph you wrote. Yes, reflowLayout should be called on every screen resolution change. GuiManager takes care of that. What I don't understand is what you mean with "engines" calling a "gui handler for that" -- no engine should have to do that, the GuiMaanger, together with the EventManager, takes care of that?
Anyway, back to the issue at hand: Revision 46947 causes this crash. Its commit message does not explain precisely what it fixes. So it would be good to know what *exactly* it was fixing, so that we can find a fix which does not lead to the regression at hand. If you can't remember, then we should just revert that commit, IMO.
comment:12 by , 14 years ago
Actually scrap the second paragraph, the GUI nowadays just uses the screen change id to check for resolution changes itself. In the old days, the engines had to call some function on EVENT_SCREEN_CHANGE to assure that the GUI was correctly notified about resolution changes.
Anyway what the commit fixes is described right there. Before that commit some widgets just assumed x/y/width/height to be setup properly in their constructor resulting in some really ugly crash for _sev. I never got ScummVM to crash on my own, but only noticed valgrind warnings (as eriktorbjorn did).
If you feel that removing it is fine, just do not forget to "fix" all widgets/dialogs (or rather all GuiObject subclasses) accordingly, since then they should never assume that their _x, _y, _w and _h are setup correctly in the constructor already.
Anyway this reflowLayout call itself here should be no problem at all, something in the theme parser should be wrong to supply such strange values at all....
Since it seems you are having a good idea how to fix it and I'm currently tired of tracing why the layout is incorrectly setup, I'm just assigning it to you and let you worry about it.
comment:13 by , 14 years ago
Anyway I just found the problem, (of course) the GUI wasn't already notified about the screen resolution change, when the global main menu was created....
I have to ways of fixing that attached.
gui_width_height_check.patch:
This patch saves the width/height of the currently parsed layout in GuiManager and uses that in GuiObject::reflowLayout for bounds checks instead of the current setup overlay dimensions.
gui_assure_correct_resolution.patch:
This patch, just always calls a g_gui.checkScreenChange() in GuiObject::reflowLayout and thus assures, that the correct layout is setup. (I consider this quite hacky and I don't know about possible side effects).
Feel free to use one of the above, fix it in a nicer way (i.e. notify the GUI somehow differently than via the current checkScreenChange call on every dialog setup), or do what ever you want....
by , 14 years ago
| Attachment: | gui_assure_correct_resolution.patch added |
|---|
by , 14 years ago
| Attachment: | gui_width_height_check.patch added |
|---|
comment:14 by , 14 years ago
| Owner: | changed from to |
|---|
comment:15 by , 14 years ago
Sadly the commit message just contains the same vague references to some crashes that you repeat here, i.e., no information that helps to actually figure out how to reproduce such a crash (or at least how to figure out where uninited data is used). I guess I'll just have to valgrind it again then.
The call to reflowLayout in the GuiObject constructor IMHO just papers of the actual bug; the two patches you attached then in turn paper over the bug revealed by calling reflowLayout in that constructor. Essentially, the GuiManager, OSystem and various widgets/dialogs are in an inconsistent state, which is bad. We should make sure that no such inconsistent state can crop up in the first place; and also, we should encapsulate the internal GuiObject / Widget / Dialog data better, to be able to prevent resp. detect access while it is not yet consistent / fully set up.
If we don't want to keep adding more layers of workarounds, we'll have to redesign how this all works. I'll be happy to work on that. Good chance to clean up some of this code, too ;).
Since this will amount to bigger changes, this work will not be suitable for the 1.1.0 branch. Hence we could apply gui_width_height_check.patch there as a quick workaround to the crash there.
comment:16 by , 14 years ago
As I said in my before mentioned statement, that I was also never able to reproduce the crash. IIRC it happened for _sev, when he opened up the options menu, yet more "vague" I think something about the tab contained widgets was not initialized properly (i.e. you should notice the warnings when opening the "Options" or "Edit Game..." menu).
Next the actual bug *was* that the widgets assume that the layout information is based on the current setup overlay resolution. This is of course not the case, since the GUI is not always notified about resolution changes. Thus when a resolution is changed and then a new widget/dialog is created *before* the GUI gets notified, the new widget will still get the layout information about the old resolution, but these checks assumed that it's layout information about the current OSystem overlay resolution, not the currently setup GUI resolution.
Of course you can call that an "inconsistent state" and the "real" bug. Of course with our current event handling way (which is poll based and not notification based), there is no way to prevent that except adding custom hooks or always polling events and updating the GUI before a widget/dialog is created (apart from just run currently).
I'm fine when you say now that of course everything the GUI now does to try to prevent problems caused by the missing notification system is a "workaround", though that's rather a broad sense of "workaround" IMHO.
A simple "workaround" to get rid of all those problems, would be to not do any layouting in the widget/dialog constructors and always reflow the layout when the dialog is opened. The "real fix" would be to add proper event notification support to OSystem and the clients and then we can assume that all the layout data is always based on the current OSystem resolution. On the other hand one would need to think of a nice way to update all existing (non opened) dialogs too... this would probably also be a reflow on every dialog open or keeping a list of created dialogs and reflow all of them on resolution change.
Anyway this is more a limitation about how the OSystem notifies client code about changes than of the GUI internal layout itself. Of course our GUI internal layout has much more serve problems too (i.e. no real separation between data and view, think of MVC).
Anyway all in all IMHO my patch is *not* a workaround, but rather a fix for a bug, which results from event code limitations we have.
comment:17 by , 14 years ago
Notifications or callbacks are certainly not required to solve this.
Yes, part of the problem is that we attempt to do layouting in object constructors, which is one of the things I want to change.
Another part is that the GUI layouting code is quite messy and convoluted, and for people writing GUI dialogs, it's often not easy to understand when they should do what. The fact that the GUI code does not attempt to do any kind of data encapsulation (a problem that I blame on myself for the most part), adds to this.
Anyway, it's evident that we will not be able to agree on any of this.
comment:18 by , 14 years ago
It is certainly true that no callbacks or notifications are required to solve this in case we do not want to do layouting in the constructors. I do not see any other way around them, when we would want to do layouting in the constructors, like it is currently *required* by the GUI. (I wouldn't disagree about that being a design flaw btw.)
Currently we only call reflowLayout in case a dialog is already open and the resolution changes, or a resolution change since the last runDialog (or whatever it was called...) call happened. Thus we require the dialogs to be setup correctly before they are run. Obviously apart from the user calling reflowLayout manually (or the like) the only choice to assure that, is to assure that they are correctly setup after the constructor call. Or call it *always* on runDialog and thus even trying to reflow correctly setup dialogs (this is what the current code tries to prevent, probably not a good reason, seeing what "problems" result of that though).
Anyway I'm fine with you changing the whole layout code etc. But that is IMHO not part of this bug report, thus I guess that's why we disagree on how to resolve this. My only interest is currently to get it fixed with the current code base and not with changing the whole GUI code base to a cleaner design. At least seeing how many new dialogs are written in real world these days, I'm not really convinced that it is worth to put that much work into the GUI (and even less convinced that this bug report is a real reason for that ;-).
IMHO a more reasonable motivation to redesign the current GUI code is to allow more seamless integration into the target platform or allowing to easily add/remove elements from dialogs etc. (I'm thinking about the GMM or the like for example, so that engines can add their own buttons there, like for example for the "Help" dialog of SCUMM). But this bug report as a motivation to change the code base itself is IMHO just plain overkill, sorry.
comment:19 by , 14 years ago
As I said, we won't be able to agree on most of this.
Of course this bug is not along justification to rewrite the GUI code yet again, as I already wrote, for the 1.1 branch, we should just apply a quick fix, e.g. one of the patches you added.
However, it is IMO showcasing quite nicely what is wrong with the GUI code; I already wrote enough about this hear, and I don't think there is a point in discussing this further here; I'll just repeat once more that *in my opinion* there are many, many things wrong with the GUI code. For me, this particular issue (and the change that triggered it) is just the straw that breaks the camels back. It's one more time I look at the GUI code and can't stop myself from shaking my head while reading through it. Note that a lot of the things I shake my head at I absolutely feel responsible for, too :).
comment:20 by , 14 years ago
I am still planning to look at this stuff, but LordHoto's patch settled the reported issue, so closing this.
comment:21 by , 14 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → closed |
comment:22 by , 13 years ago
Tested with the most recent version available from Cydia, this still occurs.
Did this patch ever make it in?
Worse, the save/load menu now report errors too.
Widget <ScummHelp.Close> has x + w > 256 (268)!
Widget <GlobalConfig.Ok> has x + w > 256 (308)!
Widget <SaveLoadChooser.Delete> has x < 0 (-12)!
comment:23 by , 13 years ago
Aah, you can ignore my latest comment the Cydia build is dated 18th December 2010.
comment:24 by , 5 years ago
| Component: | --Unset-- → GUI |
|---|
![IMG_0067[1].PNG](/attachment/ticket/4609/IMG_0067%5B1%5D.PNG){kind=link}
![IMG_0068[1].PNG](/attachment/ticket/4609/IMG_0068%5B1%5D.PNG){kind=link}
Options crash