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

Stun events leak to unrelated games #7778

Open
ProditorMagnus opened this issue Jul 9, 2023 · 10 comments
Open

Stun events leak to unrelated games #7778

ProditorMagnus opened this issue Jul 9, 2023 · 10 comments
Labels
Bug Issues involving unexpected behavior. Confirmed Issues that have been successfully reproduced by at least one developer. Regression Issues that were not present in previous releases. WML Issues involving the WML engine or WML APIs.

Comments

@ProditorMagnus
Copy link
Contributor

Game and System Information

  • Version: 1.17.17

Description of the bug

After starting new game without any units with stun and using inspect, event list includes

__empty_lua_event=yes
first_time_only=no
name="attacker_hits"
nonserializable=yes
[lua]
	code="<function>"
[/lua]
[filter_attack]
	special_type_active="stun"
[/filter_attack]

Steps to reproduce the behavior

No response

Expected behavior

Event is included to game only when there is reason for it, such as unit placed with that ability.

Additional context

No response

@ProditorMagnus ProditorMagnus added the Bug Issues involving unexpected behavior. label Jul 9, 2023
@gfgtdf
Copy link
Contributor

gfgtdf commented Jul 9, 2023

Does it actually break the implementation? If it's really just function="<code>" i don't see how it could work.

@ProditorMagnus
Copy link
Contributor Author

I would assume it was tested when this version of event was created. Whether it works or not doesnt matter for this issue context.

@Wedge009 Wedge009 added Confirmed Issues that have been successfully reproduced by at least one developer. WML Issues involving the WML engine or WML APIs. labels Jul 9, 2023
@Wedge009
Copy link
Member

Wedge009 commented Jul 9, 2023

I don't know the significance of the code="<function>" events, but it looks like they were being introduced from at least 1.17.10 (I compared with current 1.16.9 release, starting with a fresh Tutorial campaign and inspecting the events). There is a seemingly irrelevant die event with this lua code.

For the stun-filtered attacker_hits and defender_hits events, these seemed to have been introduced between 1.17.12 and 1.17.13, at cc40d5c by @CelticMinstrel.

It looks like it comes from the move of the stun lua code into data/lua/stun.lua. So maybe it's necessary to make the merfolk units part of the core even if the event leak is not desirable?

@Wedge009 Wedge009 added the Regression Issues that were not present in previous releases. label Jul 9, 2023
@Wedge009
Copy link
Member

As of 1.17.18+dev (71e83f7), I see four additional events over 1.16.9 release:

My guess is the <function> is just how the inspect module renders the lua code but is otherwise functional. Perhaps the question specific to this issue is how lua functions should be rendered in the inspection dialogue, if at all. But the title of #5663 appears to suggest that events should include lua code.

@gfgtdf
Copy link
Contributor

gfgtdf commented Jul 10, 2023

Oh right, I totally didn't see that this is just about inspect and not about savefiles.

Then I'd say this is not a problem. Maybe hide it from Inspect if it's annoying to have it there?

@ProditorMagnus could you please elaborate why you think that this is a problem?

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Jul 10, 2023

Does it actually break the implementation? If it's really just function="<code>" i don't see how it could work.

That's not the literal definition of the event. The event is registered with the Lua API (wesnoth.game_events.add). The gamestate inspector does its best to try and serialize it but is unable to decompile Lua code, so you get the string "<code>" instead.

Anyway, why exactly is this an issue? The stun event is clearly filtered so that it'll only trigger when a unit with stun attacks. It may be present in all games but it won't ever trigger if you don't have a stun unit. (If it did, that would be a bug.) I'm very tempted to close this as "won't fix".

@ProditorMagnus
Copy link
Contributor Author

I suppose its acceptable then. But when it is global event it should be at least documented that stun is reserved identifier now.

@CelticMinstrel
Copy link
Member

What do you mean by "reserved identifier" in this context?

@ProditorMagnus
Copy link
Contributor Author

https://wiki.wesnoth.org/AbilitiesWML#The_.5Bspecials.5D_tag Any other tag is valid, but will result in a special that does nothing but report it is there.

Since stun will now be active no matter if its unit is in game, it should be listed.

@CelticMinstrel
Copy link
Member

CelticMinstrel commented Jul 10, 2023

Ah, that's what you're talking about. Sure, it could be listed there. It should maybe also mention it's defined in stun.lua; that only matters to someone making a custom core, but still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues involving unexpected behavior. Confirmed Issues that have been successfully reproduced by at least one developer. Regression Issues that were not present in previous releases. WML Issues involving the WML engine or WML APIs.
Projects
None yet
Development

No branches or pull requests

4 participants