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

Fix McvManagerBotModule spamming deploy orders #17321

Merged
merged 1 commit into from Nov 20, 2019
Merged

Conversation

@abcdefg30
Copy link
Member

abcdefg30 commented Nov 11, 2019

Removes the 'activeMCVs' list since it was not useful.
The real bugfix is not iterating over 'activeMCVs' when issueing new orders (this was previously needed for already discovered mcvs that stopped) but over 'newMCVs' instead.
See this line:


On bleed the mcv manager will do something like this for new mcvs:
grafik

I suggest testing this by observing a bot with a new mcv that is stopped a few times so you can see the fixed version does actually still issue orders to it. You can use a script like this:

WorldLoaded = function()
	local player = Player.GetPlayer("Multi0")
	Trigger.AfterDelay(DateTime.Seconds(5), function()
		local mcv = Actor.Create("mcv", true, { Owner = player, Location = CPos.New(40,80) })
		Trigger.AfterDelay(DateTime.Seconds(5), function()
			mcv.Stop()
		end)
		Trigger.AfterDelay(DateTime.Seconds(10), function()
			mcv.Stop()
		end)
	end)
end
Removes the 'activeMCVs' list since it was not useful.
The real bugfix is not iterating over 'activeMCVs' when issueing new orders
(this was previously needed for already discovered mcvs that stopped)
but over 'newMCVs' instead.
@reaperrr reaperrr mentioned this pull request Nov 17, 2019
Copy link
Member

Mailaender left a comment

Looks sane.

@reaperrr reaperrr merged commit 980c1e1 into OpenRA:bleed Nov 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr reaperrr mentioned this pull request Nov 20, 2019
12 of 19 tasks complete
@abcdefg30 abcdefg30 deleted the abcdefg30:AImcvs branch Nov 20, 2019
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 22, 2019

@Inq8 suggested in Discord that this may have introduced a significant performance regression. Can we get some more testing here?

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Nov 22, 2019

I really can't believe this is causing anything significant (we actually only reduced enumerations). Are you sure this isn't more likely to be caused - if at all [1] - by the second PR he linked (#16347)? ([1]: Actually I'm not even sure if I trust this method of performance measuring.)

I just ran a 10-minute AI game (6 AIs, maximum speed) and newMCVs is empty except for the start (since they didn't build new MCVs).

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 22, 2019

The Firestorm PR shouldn't be making any logic-related changes to anything in the default mods - just adding a new extension point in the base support power code which should have a negligible overhead.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Nov 22, 2019

Yes, should not, but does. It changes much of the support power related code and even take cover and the changes in general are much less clear than in this PR - if one of those is really responsible it is more likely we overlooked something in #16347 than here.

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