Abstract docking from Refinery & Harvester#20636
Merged
Merged
Conversation
Member
Author
|
Preview: SpaceAgeProcessing.mp4 |
Mailaender
reviewed
Jan 25, 2023
Mailaender
reviewed
Jan 25, 2023
Mailaender
reviewed
Jan 25, 2023
Mailaender
reviewed
Jan 25, 2023
PunkPun
force-pushed
the
abstract-DockClientBase
branch
4 times, most recently
from
8752ce5 to
86f5240
Compare
PunkPun
force-pushed
the
abstract-DockClientBase
branch
7 times, most recently
from
d0a30de to
7dfff17
Compare
AspectInteractive2
suggested changes
Jan 28, 2023
Contributor
AspectInteractive2
left a comment
AspectInteractive2
left a comment
There was a problem hiding this comment.
Overall this is very well designed and cleanly written, I only made some minor suggestions to make this a bit more extensible and tidy up a few parts.
AspectInteractive2
suggested changes
Jan 28, 2023
Contributor
AspectInteractive2
left a comment
AspectInteractive2
left a comment
There was a problem hiding this comment.
Same feedback as with my prior review, just a few extra proposed changes.
PunkPun
force-pushed
the
abstract-DockClientBase
branch
3 times, most recently
from
4838c2f to
e23d2d4
Compare
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
cbd81b3 to
c1a3ef6
Compare
Mailaender
reviewed
May 20, 2023
Mailaender
previously approved these changes
May 20, 2023
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
c1a3ef6 to
71cfd1a
Compare
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
71cfd1a to
1fc6f6e
Compare
Member
|
Please remove the test case. 1fc6f6e |
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
1fc6f6e to
eaebe67
Compare
Member
Author
|
Dropped testcase and rebased |
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
eaebe67 to
60ba8e1
Compare
Member
|
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
60ba8e1 to
5b46d5e
Compare
Member
Author
|
It was a rebase error, fixed by squashing 4th commit into the 3rd |
penev92
reviewed
Aug 2, 2023
| return true; | ||
| } | ||
|
|
||
| public virtual bool DockingPossible(BitSet<DockType> type, bool forceEnter = false) |
Member
There was a problem hiding this comment.
IsDockingPossible. Boolean members should start with is or similar.
Also CanDock and IsDockingPossible seem redundant to each other.
Member
Author
There was a problem hiding this comment.
IsDockingPossible. Boolean members should start with is or similar.
sure
Also CanDock and IsDockingPossible seem redundant to each other.
They are completely different, you can read the documentation or just check out DockClientBase
penev92
reviewed
Aug 2, 2023
| public interface INotifyDockClient { void Docked(Actor self, Actor host); void Undocked(Actor self, Actor host); } | ||
|
|
||
| [RequireExplicitImplementation] | ||
| public interface INotifyDockClientMoving |
Member
There was a problem hiding this comment.
Why is this not part of INotifyDockClient?
Member
Author
There was a problem hiding this comment.
Because Harvester (nor other clients) are not supposed to inherit it. It'a a notify inferface, meant for trait that wants to be notified
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
5b46d5e to
f26b598
Compare
Redundant since 2013 PR: # 3407 Commit: 1eb04a7
HarvesterDockSequence -> GenericDockSequence DeliverResources -> MoveToDock
PunkPun
force-pushed
the
abstract-DockClientBase
branch
from
f26b598 to
6408dd5
Compare
Mailaender
approved these changes
Aug 8, 2023
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #20288
Closes #16464
Split from #20168
Supersedes #20181
Replaced search by actor name to search by
BitSet<DockType>Linking refactored and renamed to reserving
Abstracted
DockHost:IDockHostfromRefineryAbstracted
DockClientBase<>:IDockClientandDockClientManagerfromHarvesterAbstracted
INotifyDockClientMovingfromINotifyHarvesterActionThere are still a lot of features than need to be added to this codebase for it to be able to absorb Rearm / Repair docking.
I advise to review commit by commit as git diff doesn't deal with file renames well