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

Clean up firing data #14

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Clean up firing data #14

wants to merge 5 commits into from

Conversation

BarmonHammer
Copy link
Member

No description provided.

Comment on lines +231 to +235
pub impact_damage: f64,
#[wasm_bindgen(js_name = "ExplosionDamage", readonly)]
pub explosion_damage: f64,
#[wasm_bindgen(js_name = "CritMult", readonly)]
pub crit_mult: f64,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naive question: will you ever be able to get away from floats, are you stuck with them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what alternative would you suggest? something like the rust_decimal crate?
i'll be doing a pretty big changes soon, so if the need is there i don't mind throwing that in


if !_pvp {
let persistent = crate::PERS_DATA.with(|_perm_data| _perm_data.borrow().clone());
out.apply_pve_bonuses(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the idea that you'd calculate a base value of damage and subsequently apply multipliers for different activities?

If so, the approach I might suggest to make _pvp not a boolean, but rather, an enum that currently contains PvE and PvP. Since they're going to be adding additional PvP modes with different TTKs in season 22, I think might be worthwhile to get ahead of this while you're changing things here.

@BarmonHammer BarmonHammer added the breaking This is a breaking change and requires changes on frontend label Sep 16, 2023
@oh-yes-0-fps
Copy link

@BarmonHammer we can pick this back up while removing the breaking changes for kat if that's holding it up, we should probably start supporting a v2 API that we can rapidly iterate on and test while trying to keep it as close to the current one, then kat can update as she wants if ever.

Any type of dps simulation will need to change how the user interacts with weapons so might as well start now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change and requires changes on frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants