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

HarvesterBotModule should not command harvesters that cannot be ordered #20465

Merged
merged 1 commit into from Nov 24, 2022

Conversation

dnqbob
Copy link
Contributor

@dnqbob dnqbob commented Nov 19, 2022

When removing units "that cannot be ordered", we remove units that are dead/in world (see line 97), but later when we search new units, we add them back (see line 105). We should avoid add them back.

@Mailaender
Copy link
Member

Does world.ActorsHavingTrait<Harvester>() really give you dead harvesters that are not in the world?

@PunkPun
Copy link
Member

PunkPun commented Nov 19, 2022

The whole module feels extremely sketchy. The resource search activity it tries to control is also probably the most performance intensive in the engine

@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 19, 2022

Does world.ActorsHavingTrait<Harvester>() really give you dead harvesters that are not in the world?

I find this in my sync-report

Orders Issued:
	 ClientId: 1 OrderString: "Harvest" 
	 Type: "Fields".  
	 Subject: "cabharv 1551 (not in world)". 
	 Target: "92672,68056,0".
	 TargetString: "".
	 IsImmediate: False.
	 Player(PlayerName): Shattered AI 2

The harvester is not in world but it is in carryall

@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 19, 2022

The whole module feels extremely sketchy. The resource search activity it tries to control is also probably the most performance intensive in the engine

Extractly. I also link an old issue from CA here:
Inq8/CAmod#23

This module used to cause huge perf loss when minefield was near confliction point

Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't hurt.

@Mailaender
Copy link
Member

This probably effects Dune 2000 as well.

@abcdefg30 abcdefg30 merged commit 6fbdc2c into OpenRA:bleed Nov 24, 2022
@abcdefg30
Copy link
Member

Changelog

And picked to prep since it's a simple improvement:
d55af8d

@dnqbob dnqbob deleted the harvbot branch November 25, 2022 00:39
@PunkPun
Copy link
Member

PunkPun commented Nov 25, 2022

related #18198

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants