-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
Replace the Man class with the classes that fsmDanger is applied to
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 |
I agree moving it away from the |
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 |
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 The change here for sensitivity originates in the class 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. |
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? |
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. |
When merged this pull request will: Title
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.
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.