[Rseding] [1.1.30] Opened GUI and controller change edge case

[Cooldude2606]

While working on a spectator feature for a scenario, I discovered an edge case where "player.opened" is nil during and after the "on_gui_opened" event.
This edge case occurs when setting a new value for "player.opened" and the "on_gui_closed" event changes the players controller type.
I believe this is a bug because at no point is the value of "player.opened" set to the new value (not during close, during open or after).
I have found a work around for now which is to set "player.opened" to nil before setting your new value. (see ln70 in sample)
This work around suggests that the issue only exists when close and open are called during the same operation.

In the provided sample the case can be reproduced with the commands "/spectate" then "/display message":

Sample Code

Code: Select all

-- Destroy a gui element if it is valid
local function destroy_if_valid(element)
	if element and element.valid then element.destroy() end
end

-- Print the currently opened gui element name
local function print_opened(player, msg)
	player.print(msg..tostring(player.opened and player.opened.name))
end

-- Print if the player is in spectator mode
local function print_spectator(player, msg)
	player.print(msg..'Spectator? '..tostring(player.controller_type == defines.controllers.spectator))
end

-- Put the player into spectator
local function start_spectate(player)
	local character = player.character
	player.set_controller{ type = defines.controllers.spectator }
	player.associate_character(character)
	-- Add a message to the screen
	print_opened(player, 'Before Spectate: ')
	player.opened = player.gui.center.add{
		name = 'spectate_label',
		type = 'label',
		caption = 'You are in spectator'
	}
	print_opened(player, 'After Spectate: ')
end

-- Take the player out of spectator
local function stop_spectate(player)
	local character = player.get_associated_characters()[1]
	if character and character.valid then
		-- Give the player their original character
		print_opened(player, 'Before Controller Change: ')
		player.teleport(character.position, character.surface)
		player.set_controller{ type = defines.controllers.character, character = character }
		print_opened(player, 'After Controller Change: ')
	else
		-- Respawn the player
		player.ticks_to_respawn = 300
	end
	-- Close the gui if it is present
	destroy_if_valid(player.gui.center.spectate_label)
end

-- Command will toggle spectator while preserving your character
commands.add_command('spectate', 'Toggle spectate', function(event)
	local player = game.get_player(event.player_index)
	if player.controller_type == defines.controllers.spectator then
		player.print('## Left spectator')
		stop_spectate(player)
	else
		player.print('## Entered spectator')
		start_spectate(player)
	end
end)

-- Command will display a message on your screen
commands.add_command('display', 'Display a message on the screen', function(event)
	local player = game.get_player(event.player_index)
	destroy_if_valid(player.gui.center.display_label)
	if not event.parameter then return end
	-- Add the new message if one is given
	player.print('## Display new message')
	print_opened(player, 'Before Display: ')
	print_spectator(player, 'Before Display: ')
	-- player.opened = nil -- Uncomment for work around
	player.opened = player.gui.center.add{
		name = 'display_label',
		type = 'label',
		caption = event.parameter
	}
	print_opened(player, 'After Display: ')
	print_spectator(player, 'After Display: ')
end)

-- Pressing esc will exit spectator and dismiss the display message
script.on_event(defines.events.on_gui_closed, function(event)
	local element = event.element
	if not element or not element.valid then return end
	local player = game.get_player(event.player_index)
	player.print('# Closed: '..element.name)
	print_opened(player, 'During Close '..element.name..': ')

	if element.name == 'spectate_label' then
		-- Handle the spectate label
		stop_spectate(player)
		destroy_if_valid(element)
	elseif element.name == 'display_label' then
		-- Handle the display label
		destroy_if_valid(element)
	end

end)

-- Only used to print events to chat
script.on_event(defines.events.on_gui_opened, function(event)
	local element = event.element
	if not element or not element.valid then return end
	local player = game.get_player(event.player_index)
	player.print('# Opened: '..element.name)
	print_opened(player, 'During Open '..element.name..': ')
end)

The output (in chat) from this sample is shown below. The right hand image shows the output with the work around.

Sample Output

[Klonan]

I have narrowed it down, it really isn't anything to do with the events,

Basically, changing the controller will nil out the player.opened

factorio-run_2021-04-15_10-43-32.png (36.13 KiB) Viewed 5264 times

I will leave the rest to Rseding to decide whether it is a bug or not

[Rseding91]

player.opened getting cleared when switching controllers is intended. No event is sent in that case. The same happens when the target is invalidated (for example the opened entity is killed, destroyed, or just removed through some other means). The event only fires in reaction to the player closing the GUI himself.

I'm not seeing any other weirdness here - unless I missed something. If I did please let me know. Otherwise this seems to be working within the limitations of what the code can do.

[Cooldude2606]

I understand that changing controller will set opened to nil; however, opened is already nil during on_gui_closed and when set_controller is called.
(This is seen in my event output with "During Close spectate_label: nil" and "Before Controller Change: nil")
Therefore, the controler change during the closed event should have no impact on the final value of opened, since the new value has not yet been set.
But it does effect the final value, which causes opened to be nil during on_gui_opened and afterwards; which is why I reported this issue.

[Rseding91]

I'm having a hard time understanding what you're saying. Can you phrase it in this format?

*what happens*

*what you expect to happen*

So I can reproduce it locally and see why what you expect to happen isn't.

[Cooldude2606]

What to do
Assign a LuaGuiElement to player.opened.
Assign a different LuaGuiElement to player.opened.
During on_gui_closed make a call to LuaPlayer.set_controller.
(I am not sure if the type of controller has an affect)

What happens
on_gui_closed is raised with the first gui element. (as expected)
During on_gui_closed, player.opened is nil. (as expected)
Before the call to set_controller, player.opened is nil. (as expected)
After the call to set_controller, player.opened is nil. (as expected)

on_gui_opened is raised with the second gui element. (as expected)
During on_gui_opened, player.opened is nil. (not expected)
After the assignment, player.opened is nil. (not expected)

Expected
During on_gui_opened, player.opened should be the second LuaGuiElement.
After the assignment, player.opened should be the second LuaGuiElement.

Why is this expected
When set_controller is called, the value of player.opened is already nil. Therefore, the nil assignment from set_controller should have no affect.
When "player.opened = nil" is used in the place of set_controller, the expected result occurs, suggesting the issue lies within set_controller.
When "player.opened = nil" is used before the assignment, the expected result occurs, suggesting the issue lies with the events being called together.
(Assigning nil before assigning my second gui element is my current work around)

[Klonan]

It is the same thing, you are basically invalidating the controller during the assignment,
The code

Code: Select all

player.opened = player.gui.center.add(label)

Internally is something like this

Code: Select all

controller = player.get_controller()
new_gui = make_lua_gui(label)
if controller.has_open_gui()
  controller.close_gui({send_events = true})
end
controller.open_gui(new_gui)

However in your case, you set the player to a new controller in the 'close_gui()',
So the 'controller.open_gui()' happens on the old controller.

[Rseding91]

Ok, based off what Klonan showed I found the issue and it's now fixed. The fix however is that the "gui opened" event with nil .opened just doesn't happen anymore. Since as he showed; you invalidate the controller during the closed event and so it can't set the new .opened after that happens.

[Cooldude2606]

Thanks for the fix and sorry I wasnt able to explain it well the first time.