Hi, Guys, and @michalis !
Here is my repo with small side project that I wanted to improve from old version :
Today I made couple of changes (but maybe they were not the real reason for the bug, maybe even before it was there, so I think previous version could be also tested to reduce amount of code to be analyzed, however it is just a start of a project so there is not so much code either).
The bug is as follows: From mainMenu view I setup menubuttons registering motion and click events, everything works fine when menu starts on gamestart. When I switch to other view by PushView (in this case game view) and then again to next view (defeatview, the same way with PushView), I want like logic to return back to mainmenu from defeat view. When I was using just assignment Container.View := MainView , buttons in main menu give an error in Motion event. I tried also make PushView(MainMenu) from defeatview and error dissappears but behavior of buttons still wrong, no pictures shown and no proper Click reaction (for example Options panel blinks but doesn’t stay).
So I tried couple of simple attemps to fix in my code, but not luck for now, esp. as I see only callstack from Castle code.
So please who has some time, test and confirm or propose ideas, maybe I didn’t really properly setup/desetup the events, or I need more precise control of start/stop/resume and such methods.
Thanks !
Hello, I show your code. In GameViewMain, you use the same event for the most buttons. In this event you wrote:
procedure TViewMain.ButtonMotion(const Sender: TCastleUserInterface; const Event: TInputMotion; var Handled: Boolean);
begin
if SelectedButton = Sender then Exit;
if SelectedButton <> nil then
SelectedButton.ImageScale := 0;
SelectedButton := Sender as TCastleButton;
SelectedButton.ImageScale := 1;
if SelectedButton <> ButtonOptions then
GroupOptions.Exists := False;
end;
Here:
SelectedButton := Sender as TCastleButton;
You copy the address of the Sender (which is a TCastleButton) to SelectedButton, so SelectButton points to the Sender(no copy), no problem, But you never create the SelectedButton, so I think sometimes you are pointing “nothing” to a TCastleButton.
So I add to Start procedure this line:
SelectedButton:= TCastleButton.Create(FreeAtStop);
I tested a few times, and I didn’t got any error.
I’m not expert (may be I’m wrong or I explain bad)
Hi, Bluelcaro !
Your advice was very helpful, I understood the issue.
The thing that SelectedButton is just a reference, there is no need to create it (actually it is wrong to create it, because it would lead to memleak without freeing it). But your example made me a clue to clear this reference (which I was not doing before), it was non-null in the case of main menu view restart/reopen, but a dangling pointer which caused this error. And once I did it, it works fine now, thanks a lot ! And sorry for this stupid mistake, I should have taken first a fresh look the next day before posting, but I did it hastily in the night .
The thing that SelectedButton is just a reference, there is no need to create it (actually it is wrong to create it, because it would lead to memleak without freeing it).
I thought that can be a leak, So the owner is FreeAtStop, which is a CGE-owner, which takes the responsibility of free (@michalis I’m wrong?). So I tested the my code in Lazarus in Debug Mode and I got any leaks.
But As you said my path works but is not 100% correct. So I show your commit, and I learned a new thing.
When I was a teenager, a teacher told us: The best way to improve your knowledge is explaining it to yours classroom mates.
If the owner is FreeAtStop but if we never hit this Stop, recreation (in general, not namely in Start) would overwrite the variable thus giving memleak. But in this case you are probably right, because Start would be called only in case or fresh start, when we know that Stop was called before (when first work with the view was done)
Indeed, the SelectedButton was a dangling pointer in this case. Clearing it every time when the view starts (from Start or even from Resume, like fixed the dangling pointer of selectedbutton in menu · phomm/Towerfight@cb895bb · GitHub ) is indeed one solution. I would actually recommend to do it (SelectedButton := nil) from TViewMain.Stop or TViewMain.Pause overrides, to clear it sooner. Doing it in TViewMain.Stop seems “most correct” to me, because the button will be indeed destroyed right after TViewMain.Stop.
To explain more what happens:
There is 1 and only 1 instance of each “view”, created at the beginning of the application.
Each view can be “started” or “stopped” multiple times during the application lifetime. The designed components (from xxx.castle-user-interface) are created right before Start and destroyed right after Stop. The components owned by FreeAtStop are also destroyed right after Stop.
When one does Container.PushView(NewView), then the old view remains in “started” state. It means there is a “view stack”, with old view behind, and new view in front.
However, when one does Container.View := NewView, then the old view gets stopped.
So, what happened:
TViewMain starts (designed components from gameviewmain.castle-user-interface are instantiated, TViewMain.Start gets called).
User moves the mouse, so TViewMain.ButtonMotion gets called, ViewMain.SelectedButton gets set to something non-nil
User clicks the button calling TViewMain.ButtonStartClick, so Container.PushView(ViewGame) is done, so now ViewGame.Start is called. The “view stack” contains 2 views now: ViewMain behind, ViewGame on top.
ViewGame calls Container.PushView(ViewDefeat), so then we have “view stack” with 3 views: ViewMain, ViewGame, ViewDefeat.
ViewDefeat calls Container.View := ViewMain. This means that all views in the stack stop, and then ViewMain will start (again). So ViewDefeat.Stop is called, ViewGame.Stop is called, then ViewMain.Stop is called, and all UI in ViewMain gets destroyed. At this point SelectedButton becomes a dangling pointer, and should not be used again.
Before the fix, SelectedButton was mistakenly used. SelectedButton.ImageScale := 0 was accessing a property of an already-destroyed instance in this case.
After the fix, SelectedButton will be nil at the nearest Resume (which happens right after Start), so no problem.
Thanks, @michalis for detailed explanation !
I would say I knew 90% of this and used these approaches already, but usually not more than view stack size 2 (1 view in foreground and 2 on top, with clear intention to PopView back to first one).
And probably here was my confusion, when you assign View instead of Push/Pop, and Stop happens for all of the views in stack, I could’ve thought that the view that is assigned is not Stopped and then started. I mean we are like throwing away the rest of the stack and bringing the needed view just on top, why would we bother to stop-start it ? This is just my confusion, nothing like an argue.
Now this specific case is clearer and I would take this into strong consideration, thanks !
I thought in the past about alternative approach to “views” where the view is actually created when it starts, and destroyed when it stops.
This would avoid the need to clear anything “dangling after stop”. This approach has been implemented in CGE, can be activated by CreateUntilStopped, demonstrated in castle-engine/examples/user_interface/zombie_fighter_alternative_short_lived_views at master · castle-engine/castle-engine · GitHub . However, while this avoids a certain class of issues (e.g. SelectedButton in this case would get automatically cleared, since you would have a new, clear instance of TViewMain if you would use CreateUntilStopped…) but it also introduces some new ones. When switching a view can destroy the old one – one has to be extra careful with some code.
So, ultimately, CreateUntilStopped is right now implemented but discouraged (and only used in that 1 single example). It solves some problems, it creates others But you’re welcome to test it and feedback. Maybe it’s a better idea As said, it would make the bug with SelectedButton impossible.
Indeed Container.View := Xxx stops all views, for consistency. Note that you could implement something that follows the idea “pop all views until the one I want”:
{ Switches to the target view
or "pops" the stack until the designated view is reached
(when it's already on view stack). }
procedure ChangeView(const TargetView: TCastleView);
begin
if TargetView.Active then
begin
while Container.FrontView <> TargetView then
Container.PopView;
end else
Container.View := TargetView;
end;
You could use this like ChangeView(ViewMain) to get the behavior you need, i.e. ViewDefeat would do 2x pop and get back to ViewMain, without stopping + starting ViewMain.
