-
-
Notifications
You must be signed in to change notification settings - Fork 125
[Highly Customized] Distribution click action mode #1453
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
[Highly Customized] Distribution click action mode #1453
Conversation
|
Nightly build for this pull request:
This comment is automatic and is meant to allow guests to get latest nightly builds for this pull request without registering. It is updated on every successful build. |
TaranDahl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
In theory, it should work normally in multi-players, but I haven't tested it yet. It would be great if it could work as expected. |
32c90bd to
b9569c4
Compare
|
Tested locally. Doing good. |
|
I am hesitant on how to deal with this feature. It removes a huge amount of microcontrol from the game, essentially automating half of whatever required skill before and now just requires a mouse click. |
I think all you want is a toggle? |
|
I would say that if we will merge it, a toggle is a must, because the whole existence of such a hotkey alters gameplay significantly, removing a huge amount of microcontrol requireemtn. The bigger question is where do we draw the line on what should be included in mainline Phobos and what not? In other words, what is the scope of the project? This is an exaggeration, but just to show the point that I am trying to make -- imagine suddenly a pull request appears that is intended specifically to turn YR into a whole different genre of game (say, a visual novel with no RTS stuff at all) and is not usable for purposes of regular RTS players or modders. Should we merge it at all? I don't think we should, it is, as we say, out of scope. The main question is where do we draw the line between such features that are out of scope and the features that should be merged and are in the scope? And what potential drawbacks are there if we merge it? |
Personally, I came to Phobos because everyone knows Phobos. If I get something done here, then people will know it's done and other people don't need to spend time on what I've already done. I think it is meaningful.
Negligible performance impact. |
|
If you have to decide on a scope, then we can vote on those PRs that anyone says are out of scope. |
I have an idea. First, the radius can be controlled not via hotkeys, but with a mouse. How that would work? You press a hotkey to activate distribution mode, and when you press right mouse button on the target and drag your mouse without releasing the mouse button - it displays the circle and changes the radius depending on how far you drag the cursor, so you regulate the radius with the mouse drag. Then when you release the mouse - the orders are given to every selected unit. You do that each time and effectively draw circles to give orders. This is inspired by Call to Arms: Gates of Hell game. There you can spread your infantry among covers this way, click behind covers and drag to spread people better. |
Yes, I think we still need a toggle to enable this feature, not just defining buttons. I will add later.
Personally, if someone (whether they are modders or task package authors) needs a certain feature and I happen to be interested in it, I will try to do it. Whether to merge or not is actually related to whether more people will use it. But no matter what, these features will only be pushed if there is a demand.
If it is assessed that this should not be merged, then I will close it.
As TaranDahl said, for example, if this feature does not have any impact on performance and can be disabled (assuming I have already added the switch), then I think it can be merged. |
|
The opening video is based on the initial design, which is now outdated. I will share another one when I have time. |
Coronia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per discussed with CrimRecya, it already has most of the feature done, and the requested things can be added without needing to alter the existing stuffs. Better merge this first and do further tweak if needed
|
Why was this merged? This is a T2 request, and no T2 maintainer gave approval or even reviewed the code. My feedback wasn't addressed in full and I haven't expressed any agreement with dismissing it. |
|
Since this PR is labeled as |
|
I think I've already written part of my reasons above, but to be clarified I'd like to list it one more time:
To sum up, I think it would be better to not let PR stay in the fridge for too long (this one has been almost a year). As long as its tag design is extendible, being test sufficiently and not affecting existing things if not enabled, it could be merged first and maintained further in the future with more than one people's efforts (which is also my opinion towards the New Trajectory PR, if you've read it) I do apologize for merging it without a T2 review tho and wouldn't do it next time, but please take my word into consideration cuz I think it's the better way for Phobos |
|
You seem to misunderstand how collaborative projects work. If a senior team member (let alone, project lead) tells you that a change has to happen, is necessary, or a PR is otherwise unfitting, then it has to happen. What you do not do is go around their back and force whatever you think is right into the project. If you think that your counterpart is mistaken, you can try to convince them, but ultimately it is their decision as to what happens and what doesn't happen. I understand your concerns regarding potentially long feature development times. However, it is the feature author's burden to develop the feature, request reviews and address any concerns in a timely manner. While there are not many T2 maintainers, I don't believe you have exhausted all opportunities to get a review. That said, what happened is absolutely unacceptable and you've clearly overstepped your boundaries. Everyone pushes features they are interested in, and that is normal, but forcing the issue beyond your permissions without upper management's consent is a breach of trust. |
|
Treat the majority of what @ZivDero has written above as my words too.
Talking about new trajectories PR. Seeing the difficulties with acceptance of our feedback, both me and @ZivDero were trying a diplomatic approach in how to convey it as a friendly way of resolving the issue. I stopped responding because I felt bad about the conflict and didn't have the energy at the time to continue swallowing disregard for the sake of smooth resolution of the issue. We thought that the authority of myself, @Starkku and @ZivDero, as well as other maintainers together, is at least somewhat clear, and that it does not need to be brought to the conversation, but first your responses with borderline disrespectful comments about "losing patience", "wasting time" and now what happened here yesterday (and especially that) suggests that no, I should have been direct and explicit without diplomatic niceties, so let me state it clearly now. You do not have the right to dismiss the unresolved critical feedback of other maintainers of equal or higher status, or endlessly argue about it without resolution, wasting their time and patience. Yes, this is a community project, but we still have an organizational structure, and being a community project doesn't grant you automatic rights to dismiss feedback, especially feedback about core architectural issues, like Starkku's feedback about trajectories (especially considering he's the most experienced Phobos maintainer), or decide on your own what's best for Phobos and for specific PRs. |
This reverts commit caf18a0.
|
我想说,问题的根源在于Phobos的pr审核效率仍然低下得令人发指。他们说 "losing patience" "wasting time" 不是不尊重你,他们只是在陈述事实,因为事实就是你们在轨迹PR上花费了太多的时间。 |
While I agree it is a problem, this is not what is happening here, nor it was what was happening in the #1582. The review feedback was given, but instead of being addressed it was dismissed multiple times.
That is not stating the fact. Stating the fact would be "We have spent a lot of time on this PR". What was said instead was disrespectful, when we were actively trying to be diplomatic, the other side didn't care on the same level about a choice of words.
No, the fundamental reason is the lack of active T2 maintainers and slow "growth" of new T2 maintainers, which is the cause of slow review process. Yes, it's sad that it's slow, but there's no magic to just get more maintainers or magically transfer more skills and experience to others to be able to handle the project on a higher level, and I am not willing to shoot the project in the foot by accepting code with major architectural issues. We neither lose any money, nor endanger someone's lives, nor have any other life dependency, nor do we even have deadlines to warrant that, but we do have reputation and standards to uphold, which is my responsibility to do, ultimately. |
|
And for the record, we always had issues with reviewing and housekeeping. My contribution list is so small not because I am doing nothing, but because I spent more time than anyone else reviewing things in the past, to the point of burning out multiple times, because no one ever wanted to actively review and housekeep things, but everyone wanted to write them. Credits where it's due, you guys have been more actively doing that, which is the reason we went from 140 PRs to 70 PRs, which was a huge relief, but alas, now Starkku and ZivDero are busy elsewhere, I am working on completing things related to Ares/Phobos/new spawner update for CnCNet, which means we lack people on T2+ level. It is a problem that ideally requires a solution, but what has been done is not an acceptable solution to that. |
我去看了Teams,我们有8位T2维护者。即使现在有4位因为各种原因而无法审查PR,还有另外4位。他们在做什么? |
The teams are not representative of active maintainers. For the past years I've been executing the policy of keeping historical roles of people and not removing them from the team out of courtesy for their past work. It's a hobby after all, maybe they'll want to return some day and it would be mentally easier for them to do so. See active maintainers in the README.md instead. |
|
我们当然应该记住那些曾经做过贡献的人,但我认为你将他们放在T2这个标签里是不合适的,毕竟我们需要这个标签来审核PR。你可以创建一个新的,比如"Honorable Retired"之类的。 |
|
Maybe, however, let's not stray away from the core issue. |
|
Yeah. I will discuss that with you privately later. Forget it. There's nothing more to discuss. |
I dunno? Did you ask them? Yes, in an ideal world reviewers come by themselves. But as you've said, we're very short on manpower. I have TS, Starkku has PP. However, if the authors asked either of us, I'm sure they would've received feedback. Maybe not immediately. Maybe it'd take a few weeks even. But within the lifespan of this PR, even that seems pretty short. So don't hesitate to ask directly. We're a tightly knit community, and the situation dictates that it's appropriate and even necessary. |
|
This PR has to be reopened to facilitate proper process, as we, of course, don't want to stop it from being merged eventually. |
…on click action mode (Phobos-developers#1453)"" This reverts commit 4de03c1.
[ ]Distribution Mode Spread / Filter / EnableAllowSwitchNoMoveCommandhotkey. If the behavior to be executed by the current techno is different from the behavior displayed by the mouse, and the behavior to be executed will make the techno move near the target, the behavior will be replaced with area guard. Regardless of whether or not switch hotkey is used, default behavior can be changed throughDefaultApplyNoMoveCommand.AllowDistributionCommand. The new behavior is like using the selected objects one by one to click on each target within the range.AllowDistributionCommand.SpreadMode&AllowDistributionCommand.FilterModeallow you to set spread range and target filter by hotkeys, which default toDefaultDistributionSpreadModeandDefaultDistributionFilterMode.AllowDistributionCommand.SpreadModeScrollset to true;None, it is the default behavior of the game. If the range is not zero at this time, a green ring will be displayed. You can adjust the filter mode to:Like- only targets with the same armor type (Completely identicalArmor) will be selected among the targets allocated in the range. At this time, a blue ring will be displayed.Type- only targets of the same type (like infantries, vehicles or buildings) will be selected among the targets allocated in the range. At this time, a yellow ring will be displayed.Name- only targets of the same name (or with the sameGroupAs) will be selected among the targets allocated in the range. At this time, a red ring will be displayed.AllowDistributionCommand.AffectsAllies&AllowDistributionCommand.AffectsEnemiesallow the distribution command to work on allies (including owner) or enemies target. If picking a target that's not eligible, it'll fallback to vanilla command.DistributionModein theButtonListofAdvancedCommandBarandMultiplayerAdvancedCommandBar.sidec0x.mixfiles which correspond to different sides, with the namebutton12.shp.TXT_SWITCH_NOMOVE,TXT_DISTR_SPREAD,TXT_DISTR_FILTER,TXT_DISTR_HOLDDOWN,TXT_SWITCH_NOMOVE_DESC,TXT_DISTR_SPREAD_DESC,TXT_DISTR_FILTER_DESC,TXT_DISTR_HOLDDOWN_DESC,MSG:DistributionModeOn,MSG:DistributionModeOff,TIP:DistributionModeinto your.csffile.In
rulesmd.ini:In
ra2md.ini:In
uimd.ini:2024-12-16.01-47-48.mp4