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

possible OOS casued by ai setting movement to 0 #4428

Open
gfgtdf opened this issue Oct 4, 2019 · 5 comments

Comments

@gfgtdf
Copy link
Contributor

commented Oct 4, 2019

this code

un->remove_movement_ai();

sets a unit movement to 0, it does not sync this actions. So OOS will happen if a later action depends on the amount of movement points.

The folloiung modifications is liekely to reproduce this behviour in a 1vs1 match when both player droid their sides ona standard mp 1vs1 map.

[modification]
	id="a"
	name="a"
	[event]
		first_time_only=no
		name=recruit
		[object]
			[filter]
				id=$unit.id
			[/filter]
			[effect]
				apply_to=attack
				[set_specials]
					mode=append
					[damage]
						[filter_self]
							formula="movement_left = 0"
						[/filter_self]
						id="aaa"
						add=1000
						active_on=defense
					[/damage]
				[/set_specials]
			[/effect]
			[effect]
				apply_to=new_ability
				[abilities]
					{ABILITY_SKIRMISHER}
				[/abilities]
			[/effect]
		[/object]
	[/event]
[/modification]

Note that is is somewhat unlikeley for the ai to not use the complete movement of a unit in the first palce so it might not always give an OOS error.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2019

I think the best solution here would clearly be if the ai woudl not set the movement to 0 and instead use a different marker, to mask unit it does not want to move anymore. @mattsc

@mattsc

This comment has been minimized.

Copy link
Member

commented Oct 5, 2019

I don't see how this is "clearly" the best solution. This is not removing remaining MP at the end of a move, it is specific removal of the moves (or attacks) by itself and as such is a separate AI action, as shown, for example, here. It's not just marking a unit, it's a deliberate change of the gamestate that is used by different AIs/CAs in different ways, for example for checking whether an area is already under ZoC by the units that have moved. It would be a huge effort to change all of that.

But more fundamentally, I don't see why this should be treated differently from a move or an attack. So (at least based on what I have seen so far) I think the best solution would be to make sure this action is synced, just as moves and attacks are.

@gfgtdf

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

I don't see how this is "clearly" the best solution

Well from a non-ai viewpoint, this is not only an 'illegal' move that is, a move that the player cannot do, it is, more often than the opposite, also a move that might be adverse to that side, this for example of a player that droided his side, and undroid it and noticed that movement point were lost of units that would usually still have them. Also it might break umc abilities, like for example 'the unit heals 1 hp for each unused movement point'

It's not just marking a unit

it's still used as flag that is used for internal bookeeping and calculations, and as such i think it should not really effect the gamestate.

@mattsc

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Alright, I can buy into those arguments. But then we'd have to go a step further: ai.move_full() is an 'illegal move' too, with the same potential consequences. At least on the Lua side. I do not remember off the top of my head how it is implemented in C++.

This would be quite a significant change. Changing the "book keeping" is one thing. Making sure that all the candidate actions, both in C++ and Lua, use it correctly is quite a bit of work in itself. We cannot just do an auto-replace or changing a wrapper function, as sometimes we might actually want to select units with or without moves. So if we decided to do this, I think we'd have to do something like this:

  1. Change the flagging method. Any suggestions on how to do that?
  2. Change all the CAs in mainline.
  3. Deprecate all the Lua functions using the old method, so that UMC AI creators/users will be notified that something has changed. Even then, it still has the potential for silently breaking custom Lua CAs; those that rely on MP or attacks being set to zero by other CAs, e.g. in mainline.
@mattsc

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Just to clarify, I think we need to deprecate the old functions, rather than just changing how they work internally, so that at least in cases where the UMC code uses them, the content creators will be notified. As I said, there are already cases that are essentially impossible to catch because there are so many different ways to check whether a unit has moves left.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.