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

Remove Man class usage from range addon #400

Merged
merged 3 commits into from
Jul 7, 2024

Conversation

rekterakathom
Copy link
Contributor

When merged this pull request will: Title

  1. Describe what this pull request will do
    Replaces the Man class in the range addon with the classes modified by the danger addon for consistency. And also because Man applies to all animals too, which is probably not necessary.
  2. Each change in a separate line
    Replace Man class with the classes modified by the danger addon (SoldierWB, SoldierEB, SoldierGB)

Alternatively we can also modify CAManBase, but I think consistency with the application of fsmDanger is the best option.

Replace the Man class with the classes that fsmDanger is applied to
@jokoho48
Copy link
Collaborator

We need to move it either to CAManBase or add the overwrite in Civilian_F because some mods base their Units on that and then change the side later via the side config entry. but I think the most reasonable point would be moving it to CAManBase

@rekterakathom
Copy link
Contributor Author

We need to move it either to CAManBase or add the overwrite in Civilian_F because some mods base their Units on that and then change the side later via the side config entry. but I think the most reasonable point would be moving it to CAManBase

I opted to add Civilian_F instead of using CAManBase, because I see no reason to apply the sensitivity change to classes that don't get fsmDanger too

@diwako
Copy link
Collaborator

diwako commented Jul 7, 2024

I agree moving it away from the Man class. Animals do not really need this.
However the author team just discussed this PR in detail and came to the conclusion that moving it to CAManBase is recommended. Why? Any unit that does not inherit from SoldierWB, SoldierEB, SoldierGB, or Civilian_F is basically broken in vanilla Arma, since those 4 classes inherit from CAManBase why have that attribute set in 4 different places and not in one.

@rekterakathom
Copy link
Contributor Author

I agree moving it away from the Man class. Animals do not really need this. However the author team just discussed this PR in detail and came to the conclusion that moving it to CAManBase is recommended. Why? Any unit that does not inherit from SoldierWB, SoldierEB, SoldierGB, or Civilian_F is basically broken in vanilla Arma, since those 4 classes inherit from CAManBase why have that attribute set in 4 different places and not in one.

As I said above, it's because the danger FSM is applied to these classes and not CAManBase. Using CAManBase here and subclasses for the FSM might cause modded units to get this feature but not the danger FSM, and I believe that all features of this mod should apply consistently.

https://github.com/nk3nny/LambsDanger/blob/master/addons/danger/CfgVehicles.hpp#L2

@diwako
Copy link
Collaborator

diwako commented Jul 7, 2024

That is to set the different FSMs. True if we want to make it consistent we should do it like the vanilla config and set the FSM in CAManBase and in Civilian_F.
The reason why the FSMs are put into those 4 classes is to highlight and specifically set them for each side, as the civilian side has their own config change even in vanilla.

The change here for sensitivity originates in the class Man. CAManBase inherits from that, civilians have the same values, so imo it is easier to just set them in one class and it is done.

These currently do the same things, so why not just use it in a more simplified way?

@rekterakathom
Copy link
Contributor Author

That is to set the different FSMs. True if we want to make it consistent we should do it like the vanilla config and set the FSM in CAManBase and in Civilian_F. The reason why the FSMs are put into those 4 classes is to highlight and specifically set them for each side, as the civilian side has their own config change even in vanilla.

The change here for sensitivity originates in the class Man. CAManBase inherits from that, civilians have the same values, so imo it is easier to just set them in one class and it is done.

These currently do the same things, so why not just use it in a more simplified way?

If these currently do the same things, why are you making me do the other thing?

If you insist on this, I suppose I can make the change. Though I want to say that it feels extremely nitpicky that I now have to make the effort to switch back to the "easier" solution even though I have already implemented this solution (with a fair justification as to why in my opinion), for basically zero functional difference. And nobody is going to care which way it is implemented (other than to nitpick this PR and forget about the whole thing once merged). This file had its last commit 3 years ago, it's not like this is looked at daily.

@diwako
Copy link
Collaborator

diwako commented Jul 7, 2024

The only different right now is, UBC of 4 classes vs UBC of 1 class. It is fine if you do not want to change it, I or another author can take over the pr in the future.

@rekterakathom
Copy link
Contributor Author

The only different right now is, UBC of 4 classes vs UBC of 1 class. It is fine if you do not want to change it, I or another author can take over the pr in the future.

Why does the amount of changed classes matter? We are talking singular microseconds in parsing time. This extremely nitpicking attitude in PR reviews comes across to me as unwelcoming and even borderline hostile. Do you not want pull requests?

@diwako
Copy link
Collaborator

diwako commented Jul 7, 2024

We appreciate every PR, due to long dev cycles (granted because of real life constraints) we objectively need to do harsher reviews. I am sorry if this all came across nitpicky and hostile to you. There are no hard feelings or hostility towards you.

@diwako diwako merged commit 9670c56 into nk3nny:master Jul 7, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants