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

Added buildradius checkbox to lobby options #14209

Merged
merged 1 commit into from Nov 1, 2017

Conversation

Projects
None yet
6 participants
@ghost

ghost commented Oct 18, 2017

Implements #14203

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 18, 2017

sorry about dune2k commits, I do not know alot about it

ghost commented Oct 18, 2017

sorry about dune2k commits, I do not know alot about it

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Oct 18, 2017

Member

sorry about dune2k commits, I do not know alot about it

Don't worry. ^^ Just squash the commits together later on.
(Btw, if you got questions, feel free to reach out to me or someone else on our IRC channel #openra.)

Member

abcdefg30 commented Oct 18, 2017

sorry about dune2k commits, I do not know alot about it

Don't worry. ^^ Just squash the commits together later on.
(Btw, if you got questions, feel free to reach out to me or someone else on our IRC channel #openra.)

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 18, 2017

Contributor

This is fundamentally wrong and suffers from tunnel vision - the ticket would be the perfect case to be resolved akin with a mutator, since you're now propagating a trait used in a single mod to be disabled in every shipped and thirdparty one (you can't really disable lobby checkboxes).

Contributor

GraionDilach commented Oct 18, 2017

This is fundamentally wrong and suffers from tunnel vision - the ticket would be the perfect case to be resolved akin with a mutator, since you're now propagating a trait used in a single mod to be disabled in every shipped and thirdparty one (you can't really disable lobby checkboxes).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 18, 2017

is tarvis-ci failing at getting mono usual?

ghost commented Oct 18, 2017

is tarvis-ci failing at getting mono usual?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

is tarvis-ci failing at getting mono usual?

No, this appears to be an upstream issue. I assume that they should fix it within a day or two.
In any case, the tests would fail because you're still using space indentation in a few places. Can you please fix that?

Member

pchote commented Oct 18, 2017

is tarvis-ci failing at getting mono usual?

No, this appears to be an upstream issue. I assume that they should fix it within a day or two.
In any case, the tests would fail because you're still using space indentation in a few places. Can you please fix that?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

Looks good to me, otherwise.

Member

pchote commented Oct 18, 2017

Looks good to me, otherwise.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 18, 2017

fixing in few moments, there are no spacing problems on my local files

ghost commented Oct 18, 2017

fixing in few moments, there are no spacing problems on my local files

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 18, 2017

Member

Sorry for the about face, but can you please duplicate the options yaml for ra instead of ts and keep the common copy unmodified? As @GraionDilach mentioned this probably doesnt want to be forced on downstream mods

Member

pchote commented Oct 18, 2017

Sorry for the about face, but can you please duplicate the options yaml for ra instead of ts and keep the common copy unmodified? As @GraionDilach mentioned this probably doesnt want to be forced on downstream mods

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 18, 2017

@pchote done and working

ghost commented Oct 18, 2017

@pchote done and working

@pchote

Looks better, thanks. There are a couple more minor changes you can do to improve performance:

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 19, 2017

Is this better?

ghost commented Oct 19, 2017

Is this better?

@abcdefg30

Is this better?

Yes, just some more minor things and I hope we're done with changing the code back and forth. (Sorry for the hassle, btw...)

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 19, 2017

@abcdefg30 done, I hope there is nothing else (expecially spaces)

ghost commented Oct 19, 2017

@abcdefg30 done, I hope there is nothing else (expecially spaces)

Code changed

@pchote

LGTM aside from one last minor nit:

@@ -154,7 +154,8 @@ public WVec CenterOffset(World w)
public Actor FindBaseProvider(World world, Player p, CPos topLeft)
{
var center = world.Map.CenterOfCell(topLeft) + CenterOffset(world);
var allyBuildEnabled = world.WorldActor.Trait<MapBuildRadius>().AllyBuildRadiusEnabled;
var mapBuildRadius = world.WorldActor.Trait<MapBuildRadius>();
var allyBuildEnabled = mapBuildRadius.AllyBuildRadiusEnabled;

This comment has been minimized.

@pchote

pchote Oct 20, 2017

Member

Please add an

if (!mapBuildRadius)
	return null;

as there are no base providers if the build radius is disabled.

@pchote

pchote Oct 20, 2017

Member

Please add an

if (!mapBuildRadius)
	return null;

as there are no base providers if the build radius is disabled.

This comment has been minimized.

@ghost
@ghost
@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 20, 2017

@pchote is this good?

ghost commented Oct 20, 2017

@pchote is this good?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 21, 2017

well great

ghost commented Oct 21, 2017

well great

@pchote

pchote approved these changes Oct 21, 2017

@pchote pchote added the PR: Needs +2 label Oct 21, 2017

@AndriiYukhymchak

Not a fan of this new row while there is just one item, but this works as advertised. LGTM

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 21, 2017

Member

Not a fan of this new row while there is just one item

There are two other long thought about checkboxes that could be implemented by somebody to fill in the gaps (but certainly not in this PR!):

  • Kill bounties
  • Undeployable MCV (from the original games)
Member

pchote commented Oct 21, 2017

Not a fan of this new row while there is just one item

There are two other long thought about checkboxes that could be implemented by somebody to fill in the gaps (but certainly not in this PR!):

  • Kill bounties
  • Undeployable MCV (from the original games)
@AndriiYukhymchak

This comment has been minimized.

Show comment
Hide comment
@AndriiYukhymchak

AndriiYukhymchak Oct 21, 2017

Contributor

Yeah, got it. Maybe @kosti1 can create separate pr for those, so in playtest we would have full row. Either way this is good as it is

Contributor

AndriiYukhymchak commented Oct 21, 2017

Yeah, got it. Maybe @kosti1 can create separate pr for those, so in playtest we would have full row. Either way this is good as it is

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Oct 22, 2017

will look in to it
I have already implemented bounty checkbox in my modded engine (but it might be very unoptimized)

ghost commented Oct 22, 2017

will look in to it
I have already implemented bounty checkbox in my modded engine (but it might be very unoptimized)

@ghost ghost referenced this pull request Oct 22, 2017

Closed

Implement bounty checkbox #14245

@MustaphaTR

Looks good to me 👍

@abcdefg30 abcdefg30 merged commit 6a750d7 into OpenRA:bleed Nov 1, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@abcdefg30
Member

abcdefg30 commented Nov 1, 2017

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 1, 2017

Contributor

Congratulations to everyone for creating the worst precedent in regarding to customized lobby checkboxes! @pchote, since you were interested on the other day why I believe taht modder feedback is ignored in eaxact cases - here is one.

The CnCNetv5 client used by many TS/RA2 mods apply a rules edit behind the checkbox between the main rules and the map edits. That system should have been used here, not expanding the system with made-up arbitrary stuff.

Contributor

GraionDilach commented Nov 1, 2017

Congratulations to everyone for creating the worst precedent in regarding to customized lobby checkboxes! @pchote, since you were interested on the other day why I believe taht modder feedback is ignored in eaxact cases - here is one.

The CnCNetv5 client used by many TS/RA2 mods apply a rules edit behind the checkbox between the main rules and the map edits. That system should have been used here, not expanding the system with made-up arbitrary stuff.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Nov 1, 2017

Member

@GraionDilach: I don't really understand your hostility towards this. This PR doesn't set a precident; it follows our conventions that were established in #11364 and prerequisite PRs. Asking a new contributor to implement a new low-level engine feature that completely changes our lobby options conventions doesn't make sense.

Member

pchote commented Nov 1, 2017

@GraionDilach: I don't really understand your hostility towards this. This PR doesn't set a precident; it follows our conventions that were established in #11364 and prerequisite PRs. Asking a new contributor to implement a new low-level engine feature that completely changes our lobby options conventions doesn't make sense.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 2, 2017

Contributor

What does this PR does in effect on RA rules? Let me sum it up with two lines.

FACT:
	-BaseProvider:

Where this PR does set a precedent is that lousy hardcoded lobby checkboxes which should have been implemented via custom rules are acceptable as C# code fragments and encouraged to do so. This precedent is already utilized at #14245.

There wasn't a need to completely change the lobby options besides providing a secondary method. #9422 was filed exactly for lobby options causing this level of YAML edits. Ignoring it is convenient, blowing it to out-of-proportion levels is even more convenient - especially when one decides to ignore how CnCNet TS/YR is able to utilize said system for years now.

Contributor

GraionDilach commented Nov 2, 2017

What does this PR does in effect on RA rules? Let me sum it up with two lines.

FACT:
	-BaseProvider:

Where this PR does set a precedent is that lousy hardcoded lobby checkboxes which should have been implemented via custom rules are acceptable as C# code fragments and encouraged to do so. This precedent is already utilized at #14245.

There wasn't a need to completely change the lobby options besides providing a secondary method. #9422 was filed exactly for lobby options causing this level of YAML edits. Ignoring it is convenient, blowing it to out-of-proportion levels is even more convenient - especially when one decides to ignore how CnCNet TS/YR is able to utilize said system for years now.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Nov 2, 2017

Member

OK, as for the help/feedback/attitude:

since you were interested on the other day why I believe taht modder feedback is ignored in eaxact cases - here is one.

I checked - all you said prior to merge is "this is bad". That's all the feedback.
Now, assuming you had linked to the ticket that you opened (which you didn't, so ... shame on everyone else for not making the connection?), that ticket still goes exactly ankle-deep in the issue - it more or less just says "hey guys, CnCNet use X, UT2004 uses Y; they look like this: <screenshots>". You scratch the surface of an actual explanation of implementation details but go no further.
In fact, the first time I can find you mentioning that you have an idea about how to implement it other than visually was yesterday on Discord after this PR was merged. Now that was some decent feedback, but perhaps just a tiny a bit late, no?

Now, as for the suggested implementation:

From what I've read that method sounds utterly unsupportable for more than 1 or 2 very simple mutators.
I don't know how CnCNet does it, maybe the inis there are better suited for the task, maybe it's a complete mess... but for the simplest of mutators you can easily need to have a copy of your mod's entire rules folder, just because it affects every actor you have.
So if you have 20 (again, simple mutators), you either have this folder setup in your mod:

rules_default
rules_mutator_1
rules_mutator_2
rules_mutator_3
...
rules_mutator_20

or you have these yamls in your rules folder along with all the others:

mutator1.yaml
mutator2.yaml
mutator3.yaml
...
mutator20.yaml

with each of those potentially having a 2-line entry (at best) for every actor in the mod.
I think you can see how that is going to become utterly unsupportable, unmaintainable, unmanageable...
Yes, it would be (somewhat) flexible (as long as you only want to tweak trait values, which is somewhat limited for a mutator), but the YAML overhead would be enough to completely negate any usefulness IMO.

In conclusion,

  1. I'm not saying the used approach is very good or scalable, but IMO neither is the one you're suggesting.
    and
  2. Please don't go all "I tried to help but they ignored my feedback so * them" again, since like I said, your feedback actually came after this was merged.

So how about we all do something constructive for a change and try to come up with an actually good approach together?

Member

penev92 commented Nov 2, 2017

OK, as for the help/feedback/attitude:

since you were interested on the other day why I believe taht modder feedback is ignored in eaxact cases - here is one.

I checked - all you said prior to merge is "this is bad". That's all the feedback.
Now, assuming you had linked to the ticket that you opened (which you didn't, so ... shame on everyone else for not making the connection?), that ticket still goes exactly ankle-deep in the issue - it more or less just says "hey guys, CnCNet use X, UT2004 uses Y; they look like this: <screenshots>". You scratch the surface of an actual explanation of implementation details but go no further.
In fact, the first time I can find you mentioning that you have an idea about how to implement it other than visually was yesterday on Discord after this PR was merged. Now that was some decent feedback, but perhaps just a tiny a bit late, no?

Now, as for the suggested implementation:

From what I've read that method sounds utterly unsupportable for more than 1 or 2 very simple mutators.
I don't know how CnCNet does it, maybe the inis there are better suited for the task, maybe it's a complete mess... but for the simplest of mutators you can easily need to have a copy of your mod's entire rules folder, just because it affects every actor you have.
So if you have 20 (again, simple mutators), you either have this folder setup in your mod:

rules_default
rules_mutator_1
rules_mutator_2
rules_mutator_3
...
rules_mutator_20

or you have these yamls in your rules folder along with all the others:

mutator1.yaml
mutator2.yaml
mutator3.yaml
...
mutator20.yaml

with each of those potentially having a 2-line entry (at best) for every actor in the mod.
I think you can see how that is going to become utterly unsupportable, unmaintainable, unmanageable...
Yes, it would be (somewhat) flexible (as long as you only want to tweak trait values, which is somewhat limited for a mutator), but the YAML overhead would be enough to completely negate any usefulness IMO.

In conclusion,

  1. I'm not saying the used approach is very good or scalable, but IMO neither is the one you're suggesting.
    and
  2. Please don't go all "I tried to help but they ignored my feedback so * them" again, since like I said, your feedback actually came after this was merged.

So how about we all do something constructive for a change and try to come up with an actually good approach together?

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Nov 2, 2017

Member

I think you can see how that is going to become utterly unsupportable, unmaintainable, unmanageable...

Sorry, but i can't. I agree with @GraionDilach that Mutators as CnCNet has would be way better alternative to this. I see no problem with having 20 mutator files, probably under rules/mutators folder not directly rules.

CnCNet aside even original RA2 had a similar logic, called Game Types. Only difference ypu can only appy one of those.

Plus we already have stuff like disable-player-experience or campaign-tooltip files.

Member

MustaphaTR commented Nov 2, 2017

I think you can see how that is going to become utterly unsupportable, unmaintainable, unmanageable...

Sorry, but i can't. I agree with @GraionDilach that Mutators as CnCNet has would be way better alternative to this. I see no problem with having 20 mutator files, probably under rules/mutators folder not directly rules.

CnCNet aside even original RA2 had a similar logic, called Game Types. Only difference ypu can only appy one of those.

Plus we already have stuff like disable-player-experience or campaign-tooltip files.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Nov 2, 2017

Contributor

I left a comment somewhere at @Kosti1's repo on a commit weeks ago explaining this exact same thing I'm atm pissed on but apparently it was lost in a rebase.

Contributor

GraionDilach commented Nov 2, 2017

I left a comment somewhere at @Kosti1's repo on a commit weeks ago explaining this exact same thing I'm atm pissed on but apparently it was lost in a rebase.

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