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

Warhead refactor part 1: Shared InflictDamage #16994

Merged
merged 1 commit into from Aug 27, 2019
Merged

Conversation

@reaperrr
Copy link
Contributor

reaperrr commented Aug 26, 2019

Some 'small fries' I'd like to get out of the way before the bigger stuff.
Would be nice if this didn't sit around unreviewed/unmerged for too long.

@reaperrr reaperrr added this to the Next+1 milestone Aug 26, 2019
@reaperrr reaperrr force-pushed the reaperrr:wh-refactor1 branch from 4a1047d to c178c10 Aug 26, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Aug 26, 2019

I don't know what's wrong with the first commit, anyone any idea?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Aug 26, 2019

Probably related to the abstract class. Moving the IWarhead implementation from Warhead to each of the subclasses should fix the issue.

@teinarss

This comment has been minimized.

Copy link
Contributor

teinarss commented Aug 26, 2019

Im also getting these errors:

Severity	Code	Description	Project	File	Line	Suppression State
Error	CS0246	The type or namespace name 'WarheadArgs' could not be found (are you missing a using directive or an assembly reference?)	OpenRA.Mods.Common	OpenRA.Mods.Common\Warheads\Warhead.cs	61	Active
Error	CS0539	'Warhead.DoImpact(Target, WarheadArgs)' in explicit interface declaration is not found among members of the interface that can be implemented	OpenRA.Mods.Common	OpenRA.Mods.Common\Warheads\Warhead.cs	61	Active
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Aug 26, 2019

@teinarss At least that should already be fixed with the update I pushed.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Aug 26, 2019

Probably related to the abstract class. Moving the IWarhead implementation from Warhead to each of the subclasses should fix the issue.

There's no such issue with IWarhead.Delay, though, and at least going by some tests neither making DoImpact virtual instead of abstract, nor moving the interfaces to the sublasses seems to have any effect, I always get the same 39 violations listed.

Could it be that the compiler has trouble telling the interfaces and local methods apart, since they're named the same?

From TargetDamageWarhead.
Saves a few lines and allows warheads that are not
TargetDamageWarhead-based to use it.
@reaperrr reaperrr force-pushed the reaperrr:wh-refactor1 branch from c178c10 to 4de3f9b Aug 26, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Aug 26, 2019

Updated, dropped the first commit. Explicit interfaces would be nice to have, but they aren't necessary for my follow-ups and I'm not in the mood to delay my entire roadmap over them.

@reaperrr reaperrr changed the title Warhead refactor part 1: Explicit interfaces, shared InflictDamage Warhead refactor part 1: Shared InflictDamage Aug 26, 2019
Copy link
Contributor

teinarss left a comment

Looks good

@reaperrr reaperrr merged commit 8edd202 into OpenRA:bleed Aug 27, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.