[Bug] - Type:Number - Multiple inputs break

[tomorrowevening]

Using nudgeMultiplier and range currently only uses range.

import { types as t } from '@theatre/core'

sheet
  .object('Example', {
    multiplierWorks: t.number(0, { nudgeMultiplier: 1 }),
    rangeWorks: t.number(0, { range: [-1, 100] }),
    nudgeError: t.number(0, { nudgeMultiplier: 1, range: [-1, 100] }),
  })
  .onValuesChange((values: any) => {
    console.log(values)
  })

[tomorrowevening]

I'm assuming the nudge is being applied before the range, but the range uses the original variable vs the updated-nudged variable https://github.com/theatre-js/theatre/blob/25cf1d7fe867a4f8b9d0157267e3b4667240019d/theatre/core/src/propTypes/index.ts#L258

[vezwork]

Thanks for the bug report @tomorrowevening! I'll try to take a closer look at this next week :)

[vezwork]

Hey @tomorrowevening, thanks for your patience. I tested locally with this code:

const objConfig = {
  startingPoint: {
    x: types.number(0.5, {range: [-1, 1], nudgeMultiplier: -1}),
    y: types.number(0.5, {range: [0, 1]}),
  },
  scale: types.number(1, {range: [0.1, 1000]}),
}

and it seems that I have an inverted number slider (nudgeMultiplier -1) and it is between -1 and 1 as I would expect if both range and nudgeMultiplier are working at the same time. What version of Theatre are you on? Are you able to record a video of the bug you're facing?

[tomorrowevening]

Neither work - I was using 0.5.0-rc.3, then tried 0.5.0, but am seeing it in both places...

https://user-images.githubusercontent.com/626143/197052660-033aeef2-0fa4-45a7-afe9-73d935f68338.mov

[tomorrowevening]

I'd also advice to keep the nudgeMultiplier to a positive number, inversion feels counterintuitive to UI

[vezwork]

@tomorrowevening in the code you shared in the issue, you are using a nudgeMultiplier of 1 which will not affect the nudge rate. What nudgeMultipliers are you using in the video you shared?

[tomorrowevening]

@vezwork The above video uses the code you recommended. Nudging with 1 works when I don't specify the range, when both are applied, nudge isn't used

[vezwork]

Thanks. Hmm, I'm still not sure what is causing this issue. I'm not sure what you mean that "nudging with 1 works" because a nudgeMultipler of 1 should not change the nudge speed from the default speed.

Can you confirm if using {range: [-1, 1], nudgeMultiplier: 4} changes the nudge speed for you? If it doesn't work, would you able to share a codesandbox or a GitHub repo reproducing this issue? I haven't yet been able to reproduce it on my end.

[tomorrowevening]

Same as before.

Things should work like this:

export function lerp(min: number, max: number, value: number) {
  return min * (1 - value) + max * value;
}

export function clamp(min: number, max: number, value: number) {
  return Math.min(max, Math.max(min, value));
}

export function roundTo(value: number, digits: number = 1): number {
  return Number(value.toFixed(digits));
}

type UpdateParams = {
  nudge?: number
  range?: Array<number>
}
export function update(start: number, end: number, percent: number, params?: UpdateParams): number {
  let updated = lerp(start, end, percent)
  if (params?.nudge !== undefined) updated = roundTo(updated, params.nudge)
  if (params?.range !== undefined) updated = clamp(params.range[0], params.range[1], updated)
  return updated
}

Currently it does something like this, where it doesn't account for the updated variable:

export function update(start: number, end: number, percent: number, params?: UpdateParams): number {
  const updated = lerp(start, end, percent)
  if (params?.range !== undefined) return clamp(params.range[0], params.range[1], updated)
  if (params?.nudge !== undefined) return roundTo(updated, params.nudge)
  return updated
}

[clementroche]

I'm experiencing the same

[vezwork]

Thank you for the discussion! I have finally figured out what is going wrong. The nudgeMultiplier is technically always taken into account, but when a range is present it changes the behaviour of the nudge, and the nudgeMultiplier may behave unexpectedly.

if (range && !range.includes(Infinity) && !range.includes(-Infinity)) {
  return (
    deltaFraction * (range[1] - range[0]) * magnitude * config.nudgeMultiplier
  )
}

return deltaX * magnitude * config.nudgeMultiplier

Will experiment with this behaviour and will try to put up a PR and ask y'all how it feels!

[vezwork]

If I understand correctly, y'all want a config option that sets the nudge amount to a specific value, such that if you have two different ranges, but the same nudge config, your nudge amounts will be the same in both the ranges.

[vezwork]

@tomorrowevening please check out #329 to see if it would resolve your issue. You can test an example of the new behaviour in the playground here. I added an option so the nudge will behave in a way that I believe you will like. I found out that the existing behaviour was intentional so that the nudge would be proportional to the range. So, I had to add an option nudgeIgnoreRange rather than making this the default behaviour because otherwise it results in small ranges being unusable.

[tomorrowevening]

I think you're over thinking the issue, adding an additional variable isn't necessary, just addressing the current method:

deltaFraction * (range[1] - range[0]) * magnitude * config.nudgeMultiplier

won't always necessarily return the nudged value amount if the starting value isn't the same, so if I want the number to nudge by 1, but the starting value is 0.1, then every increment will be 1.1, 2.1, etc, rather than the intended integers.

[clementroche]

I would expect to step (nudgeMultiplier) value inside the range. So for a defined [1,2] range with 0.1 nudgeMultiplier, options are 1, 1.1 ... 1.9, 2

I don't think ignoreNudgeMultiplier is necessary

[tomorrowevening]

Yea exactly, same here

[clementroche]

Also i'm questionning the choice of "nudgeMultiplier" name, why not simply "step" ?

[tomorrowevening]

"Step" seems more clear, less writing too

[vezwork]

Yes, I understand where you are coming from.

Currently, nudgeMultiplier does not act as a step when there is a range present, but it does always scale the amount of nudge from mouse movement. The current behaviour is intended and not a bug. I understand that for you both this behaviour is unexpected, unintuitive and not what you want. It was made this way so that ranges can quickly be nudged from end-to-end, no matter how large the range is; and I agree that a step option may have made more sense than a nudgeMultiplier.

Now, to avoid making breaking changes, I may not be able to remove nudgeMultiplier in favour of a step option, because other users are using nudgeMultiplier (edit: I asked below if this would be possible). I am also not able to keep nudgeMultiplier and add step because these two options would conflict. This is why I added a nudgeIgnoreRange option... With nudgeIgnoreRange, nudgeMultiplyer will behave as a step (how you both would like) even if a range is specified.

[vezwork]

@AriaMinaei what would you think about deprecating nudgeMultiplier in favour of a step option for types.number? TL;DR of the disccusion: nudgeMultiplier has unexpected, unwanted, and unintuitive behaviour when the range option is present.

[clementroche]

Well maybe we can release it in the next major version and let users know ? I'm assuiming users are aware theatre.js is still under heavy developement and api could change frequently.

[tomorrowevening]

Yea, perhaps in the next major. Both DAT.GUI and Tweakpane use step, seems an easy transition for users

[clementroche]

Leva aswell

[AriaMinaei]

Hope I'm not overthinking this but here's my 2 EUR:

• step is a type constraint. Like, if a number type has step: 1, it is an integer. It won't be allowed to be a float. If given a float, you either get an error or it'll be rounded to an integer.
• nudgeMultiplier is supposed to be controller-agnostic. It's not just about mouse drag. It'll also affect nudges in an XR controller. We can fix its behavior's bug but we can't remove it completely. We can rename it though.

Given that, do y'all need the step param as a type constraint or just as a UX modifier for nudge? (If it's just a UX modifier, then if the user manually enters a value like 0.5 when step: 1, it'll be accepted as 0.5. And then nudging from there will take it to 1.5, 2.5 and so on).

[clementroche]

Just as UX modifier would be fine imo, it make sense only using mouse. I'm ok with keeping nudgeMultiplier as it is now, what we really need is to be able to use both range and nudgemultiplier at the same time

[tomorrowevening]

Yea I agree

[clementroche]

Any news about that ?

[tomorrowevening]

I guess I'll work on it this week

[tomorrowevening]

@vezwork can you take a look at my PR?

[vezwork]

@tomorrowevening thanks for #368 :) I'm no longer working on Theatre so I can't review your PR. I'll pass it on to @AriaMinaei and @AndrewPrifer.

[AriaMinaei]

I'll review tomorrow ;)