rafalh / dashfaction

A community patch for the Red Faction (2001) FPS game
https://www.factionfiles.com/ff.php?action=file&id=6252
Mozilla Public License 2.0
61 stars 11 forks source link

Add scope and scanner mouse sensitivity modifiers #281

Open GooberRF opened 1 month ago

GooberRF commented 1 month ago

This PR adds the following new commands:

Both default to 1.0, commands provide players with a method to scale mouse sensitivity when in scopes and scanners.

Resolves #67

nickalreadyinuse commented 1 month ago

big ups

is-this-c commented 1 month ago

scope_sensitivity_modifier seems wordy. You clamp it twice. But why bother clamping at all?

GooberRF commented 1 month ago

You clamp it twice.

The clamp on input admittedly is no longer necessary - I originally clamped only there because I was printing the configured value in a confirmation notification after the player issued the command, and I wanted to ensure the printed value reflected the value that would actually be applied. It could be removed now.

Clamping in the method that applies the values is to avoid a person manually editing the registry to a value outside the allowed range.

But why bother clamping at all?

Clamping on the lower end is required to avoid the potential of division by zero. Clamping on the higher end is mainly because any higher than that would be of extremely limited value, and providing a range of what is reasonably useful helps provide an indication to the player of what might be useful to try.

The logic is the same as why fov is capped. FOV less than 75 or higher than 160 would be effectively pointless. I'm not opposed to removing the caps on these two sens commands or fov I suppose, I just don't really see the value.

is-this-c commented 1 month ago

I'm not opposed to removing the caps on these two sens commands or fov I suppose, I just don't really see the value.

I would prefer removal of any clamping. It is simpler code overall. But you have a point about FOV.

The clamp exists in 3 places. It is not needed in a console variable. Notwithstanding my suggestion is as follows:

constexpr float clamp_sens_scale(float scale) {
    constexpr const float min = 0.01f;
    constexpr const float max = 4.0f;
    return std::clamp(scale, min, max);
}

In fact it is not explained why it is not sensitivity itself instead of a scale?

I agree it is wordy, though the impact of that is effectively mitigated by tab complete.

Not so much writing but reading which can be impacted by verbosity and code is read far more than it is written.

I avoided short hands such as scanner_sens and zoom_sens (which were the commands I originally had used) based on the standard rafalh has set with most other commands - reticle_scale, spectate_mode_minimal_ui, etc. I viewed compliance with established convention as more important than being as concise as I otherwise would.

I do not think these are good analogies. scanner_sens_scale does not feel out of place imo.

Moreover I think we should keep in mind (a) his code base is old e.g. some appears hastily written and/or C style (b) C++ and core guidelines evolved in this time and (c) likewise probably rafalh as well. I believe some flexibility makes sense. BTW: The same exact criticisms (a) apply to my code base too.

GooberRF commented 1 month ago

The clamp exists in 3 places.

Thanks for the note regarding a separate clamping method - that's something in retrospect I should have thought of but didn't. Will implement that.

Fair points regarding the naming convention for the commands, and I won't argue with them. Will wait to hear rafalh's thoughts though before making any such changes to them.

In fact it is not explained why it is not sensitivity itself instead of a scale?

That was my original plan before I started looking into how the game calculates it. My original implementation simply allowed the player to set these values directly:

    static auto& scope_sensitivity_constant = addr_as_ref<float>(0x005895C0);
    static auto& scanner_sensitivity_constant = addr_as_ref<float>(0x005893D4);

This proved way too confusing for players. Scanner sensitivity is calculated at 0.25 * normal sensitivity, so that one would in theory be easy. Scope sensitivity on the other hand, is applied inversely, and the stock game value is 0.09090909. The code below from mouse.cpp shows how the scale value necessarily is being applied differently in each case.

void update_scope_sensitivity()
{
    // prevent division by zero if somehow the modifier gets set to 0
    float modifier = std::clamp(static_cast<float>(g_game_config.scope_sensitivity_modifier), 0.01f, 4.0f);
    scope_sensitivity_factor = rf::scope_sensitivity_constant * (1.0f / modifier);
}

void update_scanner_sensitivity()
{
    float modifier = std::clamp(static_cast<float>(g_game_config.scanner_sensitivity_modifier), 0.01f, 4.0f);
    scanner_sensitivity_factor = rf::scanner_sensitivity_constant * modifier;
}

The reason I went instead with each configured as scale values with base 1.0 is for consistency, simplicity, and user friendliness. This way, the code handles the heavy lifting of how to apply each, and the player is left with a simple and consistent approach. A value of 1.0 is what they are used to, and they can easily decide how to tweak it based on that understanding.

is-this-c commented 1 month ago

This proved way too confusing for players. Scanner sensitivity is calculated at 0.25 * normal sensitivity, so that one would in theory be easy. Scope sensitivity on the other hand, is applied inversely, and the stock game value is 0.09090909.

No I mean why not skip mouse_sensitivity * (0.25 * scale) and set mouse_sensitivity directly?

Also what about merging both commands into e.g. zoom_sensitivity_ratio?

GooberRF commented 1 month ago

No I mean why not skip mouse_sensitivity * (0.25 * scale) and set mouse_sensitivity directly?

That can already be set with ms X (unless I'm misunderstanding you?). The purpose of this PR is to allow the player to adjust the sensitivity specifically when scoped in (this is explained further in #67 as noted)

Also what about merging both commands into e.g. zoom_sensitivity_ratio?

I initially considered this, but decided against in an effort to provide more flexibility to players. After having played with it as-is for a while now, I firmly believe that was the right call.

is-this-c commented 1 month ago

That can already be set with ms X (unless I'm misunderstanding you?). The purpose of this PR is to allow the player to adjust the sensitivity specifically when scoped in (this is explained further in https://github.com/rafalh/dashfaction/issues/67 as noted)

The scanner sensitivity is calculated from your mouse sensitivity multiplied by a constant. You considered changing this constant. Instead you decided to multiply it. What if you inject here and write a value so your mouse sensitivity and this constant are not involved? In this method I could set ms and scanner_sensitivity_modifier to the same value and have the exact same sensitivity in both modes.

GooberRF commented 1 month ago

What if you inject here and write a value so your mouse sensitivity and this constant are not involved?

Ahh, I see what you mean now. That said, I don't like the approach, here are my thoughts on it:

is-this-c commented 1 month ago

After having played with it as-is for a while now, I firmly believe that was the right call.

Can you explain why?

If a player really wanted the identical sensitivity for base and when using the scanner, they could just set the scanner modifier to 4.0. At that point, it would be 1.0x the base sensitivity.

When one changes or queries scanner_sensitivity_modifier can you make this cmd print final sensitivity? e.g. print player->mouse_sensitivity * 0.25 * x? I think it would be useful.

GooberRF commented 1 month ago

After having played with it as-is for a while now, I firmly believe that was the right call.

Can you explain why?

Well the biggest reason is just that if flexibility can be provided to players with no downside, it is my view that it always should be. More personally though, I am a fan of keeping the scanner sensitivity stock but reducing the sniper/PR scope sensitivity slightly. Having played with this feature for a while, I would certainly miss the ability to configure them separately.

When one changes or queries scanner_sensitivity_modifier can you make this cmd print final sensitivity? e.g. print player->mouse_sensitivity * 0.25 * x? I think it would be useful.

I could, but I don't think it's a good idea. In my view it would just complicate the output and confuse ordinary users (especially because it's applied in a fundamentally different way when compared to scopes). It's not a secret how it works - "power users" can review the source and/or just ask, but the output should be suitable for a general user, and what you've suggested, I don't believe is that. As-is, scanner_sensitivity_modifier outputs a modifier integer which is both exceedingly simple for a general user to understand and is the same way it's specified for scopes.

If rafalh would like output added similar to what you described, I'm happy to do it, but unless he indicates as much, I don't plan to do it because I would view it as a negative.