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

Restore weapon range feature #314

Merged
merged 14 commits into from Jun 14, 2019

Conversation

@KJeff01
Copy link
Contributor

commented Apr 8, 2019

Just dropping this here to let people who are interested in the restoration of the optimum, short, and long ranges to be able to help test it. This effort still needs to include tweaks to weapon stats shortRange, shortHit, and maybe longHit. Otherwise the ranges/accuracy will stay the same as it is currently.

Unfortunately, it is not as simple as pasting the old values back in since past versions of the stats made weird assumptions that would lead to easily observable buggy behavior. Obviously this has massive balance implications so this might take a while. But, other than that, my initial testing seems to point towards good unit behavior.

@past-due past-due changed the title Weapon range restore [WIP] Weapon range restore Apr 8, 2019

@KJeff01 KJeff01 force-pushed the KJeff01:weaponRangeRestore branch 2 times, most recently from 68d456d to 0ced64c Apr 15, 2019

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

I am going to make long range the default for now. This keeps with the "minimum impact" nature of previous restorations like the secondary order stuff. Optimum range can also hurt the desired behavior of some AI's, like the campaign attack groups, so that will need to be addressed later.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2019

Note for testers:
The file keymap.json in the configuration directory needs to be deleted
to test the keyboard shortcuts (if it already exists).
Alternatively, create a new directory and use the --configdir option.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Range orders are pointless for sensors (unless they are commanders).
And how should they work for VTOL weapons?

If you do not intend to have three different ranges for all weapons,
the GUI should probably only show the available ones.

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Range orders are pointless for sensors (unless they are commanders).

True.

And how should they work for VTOL weapons?

What do you mean? There is nothing special about VTOL weapons in regards to range the last timed I
checked.

If you do not intend to have three different ranges for all weapons,
the GUI should probably only show the available ones.

I agree. If the long and short ranges are the same then (and thus so is implied the base accuracy over distance) it would not be necessary to show the range buttons at all. Those weapons would just fire at "long" range.

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@Forgon2100 I believe I have addressed your GUI observations.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

And how should they work for VTOL weapons?

What do you mean? There is nothing special about VTOL weapons in regards to range the last timed I
checked.

I thought that their range should depend on their altitude.
But I'm probably overthinking this.

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@Forgon2100 I believe I have addressed your GUI observations.

Yes. I'm very pleased :)
I must fix ticket:4659 (the unit order panel does not get updated when
using keyboard shortcuts), but everything else works.

Now there is the question of how to rebalance weapons.
Perhaps this should be dealt with in one or more separate pull requests,
so that this one could be pushed (for the convenience of testers).
Should there be two separate efforts for campaign and MP balancing?

// deprecated
bool scrWeaponShortHitUpgrade()
{
SDWORD player, weapIndex;

This comment has been minimized.

Copy link
@Forgon2100

Forgon2100 May 8, 2019

Contributor

The file doc/CodingStyle.md discourages using types like SDWORD.

This comment has been minimized.

Copy link
@KJeff01

KJeff01 May 8, 2019

Author Contributor

Taking inspiration from the rest of the nearby code. This is a useless wzscript function like the one below it which I added only for consistency reasons.

@@ -89,6 +89,7 @@ typedef DroidOrderType DROID_ORDER;
enum SECONDARY_ORDER
{
DSO_UNUSED,
DSO_ATTACK_RANGE, /**< The attack range a given droid is allowed to fire: can be short, long or default (best chance to hit). */

This comment has been minimized.

Copy link
@Forgon2100

Forgon2100 May 8, 2019

Contributor

Since "Long Range" is the default setting, the comment is misleading.

This comment has been minimized.

Copy link
@KJeff01

KJeff01 May 8, 2019

Author Contributor

Fixed.

@KJeff01 KJeff01 force-pushed the KJeff01:weaponRangeRestore branch from e9be13c to 133e07a May 8, 2019

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Now there is the question of how to rebalance weapons.
Perhaps this should be dealt with in one or more separate pull requests,
so that this one could be pushed (for the convenience of testers).

Sounds good to me.

Should there be two separate efforts for campaign and MP balancing?

Hmm, probably. The focus will mostly be on the multiplayer side. For campaign, I suspect the already poor balance makes any effort to perfect ranges/accuracy values is a diminishing return. So a "good enough" effort is the best we can do for now.

But first the accuracy bug (#347) needs to be dealt with. The range restoration is utterly
useless otherwise and only makes things look like they work when they do not.

@KJeff01 KJeff01 changed the title [WIP] Weapon range restore Restore weapon range feature May 8, 2019

@KJeff01 KJeff01 force-pushed the KJeff01:weaponRangeRestore branch from 133e07a to d48361e May 24, 2019

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Did a rebase and fixed a merge conflict with #337. Seems to work just the same.

@KJeff01 KJeff01 added this to the 3.3.0_GM milestone Jun 13, 2019

@Forgon2100

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Now that you have fixed the accuracy bug with #371, that PR should be
pushed to master, along with this one.
These are important patches and I don't think that they will get better
by waiting. I'm sure that players will appreciate them in the next Beta.

@KJeff01 KJeff01 modified the milestones: 3.3.0_release, 3.3.0_beta2 Jun 13, 2019

@KJeff01

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

Alright, I'll merge these PRs soon.

I included a change for adapting the base hit chance for height based shots to include short range (old issue). This is necessary because the ranges of all weapons are, currently, the same and it is possible to have steep angled shots at short range distances.

@KJeff01 KJeff01 merged commit 4b6ef8e into Warzone2100:master Jun 14, 2019

7 of 8 checks passed

LGTM analysis: Python No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
freebsd_build FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
freebsd_build FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details

@KJeff01 KJeff01 deleted the KJeff01:weaponRangeRestore branch Jun 14, 2019

Forgon2100 added a commit to Forgon2100/warzone2100 that referenced this pull request Jun 23, 2019
Update Brazilian translation
* remove unnecessary fuzzy flag
* add translation of structure limit screen title
* reinstate translation of hold, pursue and guard orders
* add translation of commander limit messages
* reinstate translation of range orders
* fix format specifiers in map transfer messages

Refs b00b17c
Refs ticket:4871
Refs Warzone2100#263
Refs Warzone2100#329
Refs Warzone2100#314
Refs Warzone2100#392
Fixes Warzone2100#402
past-due added a commit that referenced this pull request Jun 24, 2019
Update Brazilian translation
* remove unnecessary fuzzy flag
* add translation of structure limit screen title
* reinstate translation of hold, pursue and guard orders
* add translation of commander limit messages
* reinstate translation of range orders
* fix format specifiers in map transfer messages

Refs b00b17c
Refs ticket:4871
Refs #263
Refs #329
Refs #314
Refs #392
Fixes #402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.