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

Move Selection into a trait #16547

Merged
merged 1 commit into from May 31, 2019

Conversation

@evgeniysergeev
Copy link
Contributor

commented May 16, 2019

Closes #16541

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch 2 times, most recently from 65e654c to e7c0b41 May 16, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I'm not a fan of merging this option upstream as this is something that I don't see any mod aside from D2 using. Doing single selection properly probably also requires further changes, like disabling the mouse drag selection box?

A much better option would be to move Selection into a trait, like we did long ago for ActorMap, ScreenMap, etc. The D2 mod can then implement its own version with custom logic.

@evgeniysergeev

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Doing single selection properly probably also requires further changes, like disabling the mouse drag selection box?

For the moment selection box is ok. it can help to select moving unit.

@evgeniysergeev evgeniysergeev changed the title Add Single Selection Move Selection into a trait May 17, 2019

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from ee85640 to a93df1b May 17, 2019

@evgeniysergeev

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

A much better option would be to move Selection into a trait, like we did long ago for ActorMap, ScreenMap, etc. The D2 mod can then implement its own version with custom logic.

Check updated commit, it is renamed to 'move Selection into a trait'

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from a93df1b to 6ac096a May 17, 2019

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from 6ac096a to 763076e May 17, 2019

@abcdefg30
Copy link
Member

left a comment

You are using spaces instead of tabs in a few places. Please fix that.
Also, I suppose this wants an update rule somewhen, but we can settle on the implementation first.

@evgeniysergeev

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

You are using spaces instead of tabs in a few places. Please fix that.

All spaces should be already replaced with tabs.

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from 763076e to 4c05615 May 17, 2019

OpenRA.Mods.Common/Traits/World/Selection.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/TraitsInterfaces.cs Outdated Show resolved Hide resolved
OpenRA.Game/Traits/TraitsInterfaces.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Also, I suppose this wants an update rule somewhen, but we can settle on the implementation first.

This change can't be automated, and the precedent from EditorPlayer and TerrainRenderer is to mention it in the SDK update notes instead of adding a useless update rule that would also be triggered for maps etc.

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch 6 times, most recently from 95a0488 to f18e82d May 17, 2019

@evgeniysergeev

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

All suggested changes for this PR made.

@pchote pchote added this to the Next Release milestone May 24, 2019

OpenRA.Game/World.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Selection.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Selection.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/World/Selection.cs Outdated Show resolved Hide resolved

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from f18e82d to 86742f6 May 26, 2019

@evgeniysergeev

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Requested Fixups done

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch 2 times, most recently from 4b8504e to a087f99 May 26, 2019

@pchote
Copy link
Member

left a comment

LGTM after one last minor tweak:

@@ -23,6 +23,7 @@
using OpenRA.Network;
using OpenRA.Primitives;
using OpenRA.Support;
using OpenRA.Traits;

This comment has been minimized.

Copy link
@pchote

pchote May 26, 2019

Member

This using isn't needed anymore, so please remove it.

This comment has been minimized.

Copy link
@evgeniysergeev

evgeniysergeev May 26, 2019

Author Contributor

removed

@evgeniysergeev evgeniysergeev force-pushed the evgeniysergeev:single-select branch from a087f99 to fb0cd12 May 26, 2019

@pchote
pchote approved these changes May 26, 2019
@matjaeck
Copy link
Contributor

left a comment

Tested in RA.

@reaperrr reaperrr merged commit 3a30b01 into OpenRA:bleed May 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@evgeniysergeev evgeniysergeev deleted the evgeniysergeev:single-select branch Jun 4, 2019

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