Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Program start takes longer than v0.55.1 #23

Closed
ghost opened this issue Dec 13, 2014 · 19 comments
Closed

Program start takes longer than v0.55.1 #23

ghost opened this issue Dec 13, 2014 · 19 comments

Comments

@ghost
Copy link

ghost commented Dec 13, 2014

When starting the program, it takes a while (maybe 10 seconds or so) until the menu appears.

With v0.55.1 it's instant.

Game start also seems to be somewhat slower.

Tested with current git.

@vLKp
Copy link
Contributor

vLKp commented Dec 14, 2014

I see no problems here. How is the CPU usage during this time? When did this first appear?

@vLKp vLKp self-assigned this Dec 14, 2014
@ghost
Copy link
Author

ghost commented Dec 15, 2014

I'll report back once I can spare a moment for testing again.

@ghost
Copy link

ghost commented Dec 18, 2014

Do you probably have CD Playback enabled in the "Sound & Music" menu? Even with no CD inserted, the initialization can delay the program start for a few moments.

@ghost
Copy link
Author

ghost commented Dec 19, 2014

I tried it again. Seems like it's always "hanging" in SDL_Delay().

@ghost
Copy link

ghost commented Dec 20, 2014

I'll put that on my list. The game sometimes pulls a "LOADING" messagebox which usually goes by so fast nowadays that it's barely noticeable. So for these I put an artificial delay of one second for the release builds so players can at least "see" that there is a loading process going on.
It's possible that this is somehow called during initialization. If that's the case, compiling the game with the debug argument should make the issue go away (as I disabled the delay for debug builds). Either way, I'm gonna try and see what causes this. Should not be that much of a problem to find the delay call and why it takes so long.

EDIT: The delay call could also be caused by the MIDI code on Windows. During the GM_reset, there also is a short delay to make sure the reset can be fully done before playing a song. Disabling Music playback or switching to something else but MIDI music will help.

@vLKp
Copy link
Contributor

vLKp commented Dec 20, 2014

Please provide the symbolic stack trace when the program is hung in SDL_Delay.

@ghost
Copy link
Author

ghost commented Dec 20, 2014

Start screen:

#1  0xb7f246b0 in SDL_Delay () from /usr/lib/i386-linux-gnu/libSDL-1.2.so.0
#2  0x0806da5a in timer_delay2(int) ()
#3  0x08122d12 in title_handler(window*, d_event const&, title_screen*) ()
#4  0x080554f2 in window_send_event(window*, d_event const&) ()
#5  0x0806c91a in event_send(d_event const&) ()
#6  0x0806cb3d in event_poll() ()
#7  0x0806cb67 in event_process() ()
#8  0x081244a5 in show_title_screen(char const*, int, int) [clone .constprop.26] ()
#9  0x08124623 in show_titles() ()
#10 0x0804d548 in main ()

Loading a level (through "new game"):

#1  0xb7f246b0 in SDL_Delay () from /usr/lib/i386-linux-gnu/libSDL-1.2.so.0
#2  0x080ad660 in LoadLevel(int, int) ()
#3  0x080ae0c0 in StartNewLevelSub(int, int, int) ()
#4  0x080ae46c in StartNewLevel(int) ()
#5  0x080ae905 in StartNewGame(int) ()
#6  0x080d805f in do_new_game_menu() ()
#7  0x080dce10 in mission_menu_handler(listbox*, d_event const&, mission_menu*)
    ()
#8  0x080fc1d7 in listbox_handler(window*, d_event const&, listbox*) ()
#9  0x080554f2 in window_send_event(window*, d_event const&) ()
#10 0x0806c91a in event_send(d_event const&) ()
#11 0x0806d093 in key_handler(SDL_KeyboardEvent*) ()
#12 0x0806cb01 in event_poll() ()
#13 0x0806cb67 in event_process() ()
#14 0x0804d715 in main ()

Also, when the level is loaded, the program seems to globally grab keyboard input, which is extremely annoying.

@ghost
Copy link
Author

ghost commented Dec 20, 2014

The game sometimes pulls a "LOADING" messagebox which usually goes by so fast nowadays that it's barely noticeable. So for these I put an artificial delay of one second for the release builds so players can at least "see" that there is a loading process going on.

IMHO this is going a bit too far with emulation. At least it should be shorter, offset the actual loading time (in case it doesn't).

@ghost
Copy link

ghost commented Dec 20, 2014

To clarify: The one and ONLY reason I implemented this is that players are able to read the iconic "Prepare for Descent" phrase. Where I think that one second is pretty much a good timing to even recognize it. In all other places, where this specific screen is NOT visible a noticeable delay is not intended.

Also when the level is loaded, yes the program grabs the keyboard and mouse input for obvious reasons (so you can save, load, etc without your Desktop being able to catch this input first, and doing something you don't want). You can switch off the Input focus in the OPTIONS menu.

@vLKp
Copy link
Contributor

vLKp commented Dec 20, 2014

Those traces look normal. The title screen has an artificial ~3 second delay that I always blast through by hitting Enter when it appears. Even allowing that to run its full delay would not account for the 10 second delay originally reported.

The grab is automatically released when some menus are open, including the Abort Game dialog. If you need to temporarily release the grab, hit escape, but do not quit the level. Your keyboard and mouse will be ungrabbed until you dismiss the dialog.

@ghost
Copy link

ghost commented Dec 20, 2014

Yes, as Kp said, the title screens, when shown take a few seconds to proceed. That's why you can skip them. You can even skip the titles all the time with the command-line argument -notitles
I thought this delay would occur BEFORE the game does even appear. But the titles delay should have been present in old versions, too. So that's why I am pretty confused.

@ghost
Copy link
Author

ghost commented Dec 20, 2014

Also when the level is loaded, yes the program grabs the keyboard and mouse input for obvious reasons (so you can save, load, etc without your Desktop being able to catch this input first, and doing something you don't want). You can switch off the Input focus in the OPTIONS menu.

It does that even if the window is not focused. Pretty bad behavior. Are you sure you want to do something as hacky and unreliable as this, instead of just telling the user to temporarily disable window manager hotkeys? (Older versions apparently only grabbed input when fullscreened, which seems reasonable behavior, especially since fullscreen is the default.)

But the titles delay should have been present in old versions, too. So that's why I am pretty confused.

In 0.55.1 it's visible for like a half second.

@ghost
Copy link

ghost commented Dec 20, 2014

It's not a hacky approach at all. I am telling SDL to keep input focus during ingame. Period. And I do that because there are enough players not versed enough to disable their Window manager keys. As I said, you can disable this by simply switching it OFF in the OPTIONS menu. It will even be saved when you next launch the game.

If the PARALLAX and INTERPLAY screen show up for only half a second in 0.55.1 then this is definitely not the intended behavior. Of course it's understandable if you don't want them. That's what "-notitles" is for.

@vLKp
Copy link
Contributor

vLKp commented Dec 20, 2014

How are you getting the game unfocused while the grab is held?

Temporarily disabling the window manager keybindings is a bad approach because not all window managers have an easy on/off switch and of those that do, not all have a switch that can be configured by Rebirth, so the user would have to flip the toggle every time they started and stopped Rebirth. To me, opening my window manager options and toggling hotkeys on/off every time I play is far more annoying than opening a game dialog box to release the grab.

I play in windowed mode so that the game does not try to change my resolution. I like having the automatic grab when playing, since that also means my window manager bindings work when I am not in a level. If this really bothers you and you do not want to use the Options dialog that zico mentioned, recent versions also feature a command line -no-grab. It was added so that freezing the game in a debugger did not trap the console, but it can be used for any case where you want grabs suppressed for the duration of the program. Unlike the menu option that zico told you to use, there is no in-game UI to change the value of -no-grab, since it is intended for use with debuggers and the debugger can change the value for you.

@ghost
Copy link
Author

ghost commented Dec 20, 2014

How are you getting the game unfocused while the grab is held?

Resuming the debugger when descent was interrupted in the loading screen. Loading will finish, and it will grab input - even if another window is marked as active.

To me, opening my window manager options and toggling hotkeys on/off every time I play is far more annoying than opening a game dialog box to release the grab.

If these window managers even require opening option dialogs just to go out of the way, then they have bad support for games. But I'm not sure if grabbing all input by force is really a solution for this. (Yes, it's all a mess, there's no "correct" solution that works.)

I play in windowed mode so that the game does not try to change my resolution.

This is another thing SDL behaves very badly (and has for many years), and for which no clean solution is available because everyone on Linux just prefers to pile hacks upon hacks. A program should never switch screen resolution. I play descent in fullscreen mode with native resolution to avoid this.

@vLKp
Copy link
Contributor

vLKp commented Dec 20, 2014

If you use a debugger to interrupt the game, I strongly recommend using -no-grab. Otherwise you can get into trouble if the game crashes, since the debugger may freeze the game before the grab can be released, depending on your signal handling settings.

Are you proposing that all window managers should have a keybinding that disables keybindings, and another keybinding that enables keybindings (and if the latter, how should you access it once keybindings are disabled)? Or are you proposing that Rebirth needs to inform the window manager not to use some keybindings? Or something else? We have already suggested two solutions to the problem of grabbed input, one for long term use (disable it in the Options dialog) and one for one-off use (e.g. debuggers via -no-grab).

Changing screen resolution is unfortunately quite popular among Windows users, because Windows support for resolution changes is cumbersome.

@ghost
Copy link

ghost commented Dec 20, 2014

Yes, no-grab or the ingame option. It basically does the same thing, while the command-line option takes first priority. But as it may be, the input focus solution is a very good way to handle all sorts of different environments. It can be changed ingame, in the matter of seconds, no matter how good or bad your window manager is "made for gaming".

As for resolutions, I am not sure about you guys. It can be different for the Software renderer and maybe for editor, but for the main game, soon as I set a resolution, no matter if window or fullscreen, the game will not change resolutions at all for me. Tho this may be different on Windows. I don't know.

@ghost
Copy link
Author

ghost commented Dec 20, 2014

Are you proposing that all window managers should have a keybinding that disables keybindings, and another keybinding that enables keybindings (and if the latter, how should you access it once keybindings are disabled)? Or are you proposing that Rebirth needs to inform the window manager not to use some keybindings?

Both would be good solutions, but yeah, they're more hypothetical and not really feasible. Especially not this late in X11's life - let's hope Wayland has sane solutions for this. Anyway, on IceWM you can map scroll lock to enable/disable global key bindings bindings, which is quite convenient for programs which require "unusual" input.

Changing screen resolution is unfortunately quite popular among Windows users, because Windows support for resolution changes is cumbersome.

It's a completely outdated concept. CRTs are over. Descent can handle arbitrary resolutions and even different aspect ratios fine, and letting the LCD monitor scale usually leads to worse results.

@vLKp vLKp removed their assignment Sep 30, 2015
@vLKp
Copy link
Contributor

vLKp commented Apr 13, 2018

Discussion veered off-topic, and has been idle more than two years. Closing due to lack of interest.

@vLKp vLKp closed this as completed Apr 13, 2018
vLKp added a commit that referenced this issue Dec 8, 2018
Commit 88b5e61 ("Replace calls to
window_set_visible in DoPlayerDead() with stop/start_time()") switched
from hiding the game window to only stopping time, and that only if
hiding the window would have stopped time.  In multiplayer, this allows
time to continue running.  This introduced a crash if the player dies
during the mine countdown.  When the player dies, the game checks if the
reactor has been destroyed.  If so, the game immediately advances to the
next level.  That advance will try to draw the game window if it is
visible.  When the window is drawn, if time is not stopped, then game
logic runs.  Some of that logic is not prepared to deal with the
inconsistent state present when a new level is only partially loaded.
That inconsistent state then causes a crash.  Call stack:

    #11 slew_frame () at similar/main/slew.cpp:152
    #12 in d2x::object_move_one () at similar/main/object.cpp:1758
    #13 in d2x::object_move_all () at similar/main/object.cpp:1956
    #14 in d2x::GameProcessFrame () at similar/main/game.cpp:1848
    #15 d2x::game_handler () at similar/main/game.cpp:1615
    #16 in dcx::window_send_event () at common/include/window.h:116
    #17 dcx::event_process () at common/arch/sdl/event.cpp:176
    #18 in newmenu_do2 () at similar/main/newmenu.cpp:498
    #19 in newmenu_do2<dcx::unused_newmenu_userdata_t> () at common/main/newmenu.h:184
    #20 newmenu_do<dcx::unused_newmenu_userdata_t const> () at common/main/newmenu.h:190
    #21 newmenu_do<1ul, dcx::unused_newmenu_userdata_t const> () at common/main/newmenu.h:196
    #22 net_udp_wait_for_requests () at similar/main/net_udp.cpp:4563
    #23 net_udp_level_sync () at similar/main/net_udp.cpp:4607
    #24 in multi_level_sync () at similar/main/multi.cpp:3458
    #25 in d2x::StartNewLevelSub () at similar/main/gameseq.cpp:1803
    #26 in d2x::StartNewLevel () at similar/main/gameseq.cpp:2018
    #27 in d2x::AdvanceLevel () at similar/main/gameseq.cpp:1648
    #28 in d2x::DoPlayerDead () at similar/main/gameseq.cpp:1721

The root cause of this is the layering violation:
- Killing the player can have the side effect of advancing the level
- Advancing the level can have the side effect of calling multiplayer code while the level data is in an inconsistent state
- Calling multiplayer code can cause the event system to redraw the game
- Redrawing the game can cause game logic to run

Hack around this by restoring the logic that hides the game window, so
that the window is not redrawn and the game logic is not run.  This does
not fix the layering problem, but prevents crashing affected users.  To
avoid undoing the feature from the breaking commit, hide the window only
when advancing to a new level, rather than unconditionally.  A player
advancing to a new level already lacks the move-at-cold-start capability
even on successfully escaping the mine, so no functionality is lost with
this change.  Players who are dead and do not advance to a new level
retain that capability.

Fixes: 88b5e61 ("Replace calls to window_set_visible in DoPlayerDead() with stop/start_time()")
Reported-by: Ninjared <https://forum.dxx-rebirth.com/showthread.php?tid=1097>
vLKp added a commit that referenced this issue Dec 8, 2018
commit d83972d upstream.

Commit 88b5e61 ("Replace calls to
window_set_visible in DoPlayerDead() with stop/start_time()") switched
from hiding the game window to only stopping time, and that only if
hiding the window would have stopped time.  In multiplayer, this allows
time to continue running.  This introduced a crash if the player dies
during the mine countdown.  When the player dies, the game checks if the
reactor has been destroyed.  If so, the game immediately advances to the
next level.  That advance will try to draw the game window if it is
visible.  When the window is drawn, if time is not stopped, then game
logic runs.  Some of that logic is not prepared to deal with the
inconsistent state present when a new level is only partially loaded.
That inconsistent state then causes a crash.  Call stack:

    #11 slew_frame () at similar/main/slew.cpp:152
    #12 in d2x::object_move_one () at similar/main/object.cpp:1758
    #13 in d2x::object_move_all () at similar/main/object.cpp:1956
    #14 in d2x::GameProcessFrame () at similar/main/game.cpp:1848
    #15 d2x::game_handler () at similar/main/game.cpp:1615
    #16 in dcx::window_send_event () at common/include/window.h:116
    #17 dcx::event_process () at common/arch/sdl/event.cpp:176
    #18 in newmenu_do2 () at similar/main/newmenu.cpp:498
    #19 in newmenu_do2<dcx::unused_newmenu_userdata_t> () at common/main/newmenu.h:184
    #20 newmenu_do<dcx::unused_newmenu_userdata_t const> () at common/main/newmenu.h:190
    #21 newmenu_do<1ul, dcx::unused_newmenu_userdata_t const> () at common/main/newmenu.h:196
    #22 net_udp_wait_for_requests () at similar/main/net_udp.cpp:4563
    #23 net_udp_level_sync () at similar/main/net_udp.cpp:4607
    #24 in multi_level_sync () at similar/main/multi.cpp:3458
    #25 in d2x::StartNewLevelSub () at similar/main/gameseq.cpp:1803
    #26 in d2x::StartNewLevel () at similar/main/gameseq.cpp:2018
    #27 in d2x::AdvanceLevel () at similar/main/gameseq.cpp:1648
    #28 in d2x::DoPlayerDead () at similar/main/gameseq.cpp:1721

The root cause of this is the layering violation:
- Killing the player can have the side effect of advancing the level
- Advancing the level can have the side effect of calling multiplayer code while the level data is in an inconsistent state
- Calling multiplayer code can cause the event system to redraw the game
- Redrawing the game can cause game logic to run

Hack around this by restoring the logic that hides the game window, so
that the window is not redrawn and the game logic is not run.  This does
not fix the layering problem, but prevents crashing affected users.  To
avoid undoing the feature from the breaking commit, hide the window only
when advancing to a new level, rather than unconditionally.  A player
advancing to a new level already lacks the move-at-cold-start capability
even on successfully escaping the mine, so no functionality is lost with
this change.  Players who are dead and do not advance to a new level
retain that capability.

Fixes: 88b5e61 ("Replace calls to window_set_visible in DoPlayerDead() with stop/start_time()")
Reported-by: Ninjared <https://forum.dxx-rebirth.com/showthread.php?tid=1097>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant