dxx-rebirth
dxx-rebirth copied to clipboard
dxx-rebirth
Make dcx::window into a class and use C++ features
When I originally wrote the d_window struct and corresponding system it was in C, with the intention of making the different views/panes in DXX-Rebirth more object oriented (and IMO cleaner code). Due to the limitations of C this meant maintaining up to 3 pointers (d_window, UI_DIALOG and robot_dialog for example). The use of unique_ptr's makes this potentially even more complicated. C++ adds features such as subclassing and polymorphism and I would be keen to make use of these features for dcx::window, in particular so that dcx::window is a subclass of grs_canvas; UI_DIALOG, newmenu and others are subclasses of dcx::window and the specific uses are further subclasses. The trigger for this is continuing issues with the editor, which I would like to work on further. That said I also like this sort of 'tidy up'.
I would do this in a separate branch, but I am posting this idea first before I start, to get some feedback.
This sounds plausible, but I am wary of using polymorphism for this. Compilers implement it correctly, but it has a runtime cost (particularly with regard to branch prediction), so I am cautious about using it without a good reason, especially if it would be on a hot path. What areas do you think would benefit from polymorphism relative to the current design?
The number one thing I would like to achieve is combine the data structures using inheritance, to reduce pointer and heap maintenance. This appears to be where the editor is tripping up currently. I would also like to do this to move some gadget access global variables in the editor (maybe more) into those data structures.
I was also thinking of making all the window manipulation/accessing functions in window.h into class methods (mainly to remove the window_ prefix).
Another thing I was considering was adding virtual draw and event_handler methods to the window class, since I feel it makes the code cleaner than passing a function as a function parameter. If this causes an unacceptable performance hit then I will stick to the current system for this.
All of the above changes would affect the code broadly (everywhere a dcx::window and all inherited structs like newmenu are used), at least that was my intention.
Combining the data structures using inheritance would mean use of C++ constructors and destructors, rather than the explicitly defined and called creator and closing functions.
I think the performance hit would be minimal in areas where the code already uses function pointers. Virtual function calls typically require one extra dereference, which should be quite cheap compared to the cost of a call by pointer. Most compilers implement vtable handling such that these two generate roughly equivalent code:
struct A
{
virtual void f();
};
struct B
{
struct VTable {
void (*f)(B *);
};
VTable *vtbl;
};
void f(A *a)
{
a->f();
}
void f(B *b)
{
b->vtbl->f(b);
}
The use of the extra level of indirection allows all instances of B to share a single copy of B::VTable, which can become important when the number of instances of B is large, since a direct embed would store # pointers * # instances.
As part of this work, it would be nice to eliminate window_exists. I have always been uncomfortable with it, since it is designed to be passed a pointer to a window that may have been freed. Although it does not access the maybe-free pointer, it compares that address against other windows, some of which could have been allocated after the previous window was freed. In a sufficiently unlucky run, a newly allocated window could have the same address as the previously freed window, which would cause window_exists to return true when it should not.
Yes, good point. Hmm, one way to implement that would be with stricter and more widespread use of window_event_result::deleted. The event_process function (and all the handler functions called by it) would return a window_event_result instead of void.
I was considering using std::list for the window list, but after reading the definition here and how to actually get the previous/next element it seems the extra code to implement it would make it almost as complicated, if not more so, than the current system of storing next and prev in each window and having static FirstWindow and FrontWindow. Any thoughts @vLKp?
In addition, I found this issue relating to subclassing windows if I went down the path of using std::list, making even more overhead to implement...
I do not understand the concern in the linked stackoverflow post. Incrementing iterators is cheap. The latter issue, regarding derived types, is a valid concern. std::list stores the managed type in the nodes, so it is not suitable when some of the stored objects may be derived from T. An intrusive list solves that problem, but is not part of the standard library. So far, we have not yet made Boost a hard dependency, so the Boost intrusive list cannot be assumed to be available.
OK thanks for your feedback. I'll stick with the current system, but replace window_get_prev/next with prev/next methods. This will make the dcx::window argument compulsory and I think it looks a bit neater too.
I'm going through and doing similar with all the other dcx::window friend functions that have a dcx::window parameter.
Adding another TODO here - return window_event_result::close to game_handler instead of calling window_close(Game_wind), as per issue #280.
While working through removing calls to window_close(Game_wind), I found a call to multi_do_frame() in draw_automap. Could this be removed without too much impact @zicodxx?
The reason I ask is if it can go, I could remove the multi_quit_game global as well and return window_event_result::close all the way back to game_handler instead. If it has to stay, it will need to close both the automap and the game window if multi_do_frame() ends the game. This could potentially cause an issue, since game_leave_menus() actually closes the automap if it's open. I would also need to keep multi_quit_game and rely on it to pass window_event_result::close to game_handler.
This pertains to when receiving or failing to send/receive a network packet results in the game being ended, i.e. whenever multi_quit_game is set to 1.
@kreatordxx I added multi_do_frame to all the 'wait for frame' loops to keep the host packet relay as responsive as possible (no matter how many frames we render per second). Happens in three functions:
calc_frame_time()
draw_automap()
timer_delay_bound()
In those cases, we could theoretically replace multi_do_frame() with multi_do_protocol_frame() if that solves your problem, @kreatordxx. Although it should check that GM_MULTI is true and Newdemo_state != ND_STATE_PLAYBACK. That's usually covered my multi_do_frame itself.
That would still be sufficient since GameProcessFrame() still calls multi_do_frame(). And that should be left intact at least once per frame.
What I'll do is the 'it has to stay' option. I'll have multi_do_frame() return window_event_result::close all the way back to game_handler in all cases if multi_quit_game is 1. The multi_quit_game global is set back to 0 only by multi_new_game() so the window closing happens reliably.
In a separate commit I'll remove the calls to game_leave_menus() in multi.cpp and net_udp.cpp and instead rely on the return value from timer_delay2(), which will pass on the return value from multi_do_frame() - i.e. window_event_result::close if multi_quit_game is 1.
This should remove many cases of a window being closed within its own handler. I'll run these past @vLKp and you can review them as well if you like @zicodxx.
I am uncomfortable with the idea that timer_delay2 can cause the game window to be destroyed, and in particular that anyone who calls it must handle that case. I recognize that this problem already exists with the current code, but I am worried that it will become more dangerous when callers are required to propagate that return value. I would prefer to see the flow restructured such that calling timer_delay2 cannot cause the game window to be destroyed (but could, if necessary, update the game window so that it is destroyed when next it receives a message from the window system). I think such a restructuring is invasive and is not necessary for what @kreatordxx intends (and I do not expect him to do it for this patch series), but I would like to have such a restructuring on the long term plan.
My original intention was to only use the proposed return value from a single call to multi_do_frame() - the one directly in GameProcessFrame(). This could work in theory, since multi_quit_game would only be set back to 0 by multi_new_game(). I later figured it would be safer to pass on the return value in all cases for Game_wind so we don't get some unintended side effect from running code until that call to multi_do_frame(). I might just have a go at this and it might be clearer to discuss it then. I'll break this up into more commits to make review easier.
Relying on a return value from timer_delay2() to close other windows, instead of using game_leave_menus(), is an entirely different case. As above, I might just have a go and we can discuss what/whether to merge into master.
@vLKp yes, I agree with you regarding your concerns. That's also a reason why I suggested multi_do_protocol_frame() in these three cases and even that is in fact overkill. In fact what might be the most lightweight option would be to have a helper function that calls net_udp_listen() (or whatever protocol-specific listen function would be employed based on multi_protocol). That would be enough to fulfill the main use of what I tried to achieve in these cases.
Even net_udp_listen() can potentially receive a packet that dumps the player, via net_udp_process_dump. If the multi_quit_game global is kept, then just by tracking that I could (in theory at least) only need to keep the window_event_result::close return value via that call to multi_do_frame() in GameProcessFrame() (only setting multi_quit_game back to 0 when starting a new multi game as I mentioned above). This would definitely need to be verified though.