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

feat(ui): configurable field constraints for number fields in form builder #7787

Merged
merged 5 commits into from
Mar 16, 2025

Conversation

psychedelicious
Copy link
Collaborator

Summary

Allow for the min and max values for Integer and Float Node Field elements in the Form Builder to be configured.

For example, the Noise node's width field is an integer from a min of 1 and no max, with a multipleOf constraint of 8.

Let's see how that gets parsed into the UI:

  • The min is 1, but because of the multipleOf=8 constraint, the effective min is the smallest multiple of 8 that is greater than or equal to the min. So the effective min is 8.
    • Note: Prior to this PR, the UI would not consider the multipleOf when setting min value for number inputs. In this example, you'd be able to leave the min on the noise node at 1, which would error if you Invoked. So this PR resolves a that footgun.
  • There is no max for width, but we must put a number on it to have a meaningful UI widget. We set it to a constant 4294967295 (IIRC this is the max value numpy accepts for its RNG seeds). Because of the multipleOf constraint, the effective max is the largest mutliple of 8 that is less than or equal to this constant. That turns out to be 4294967288.
  • The multipleOf value is used as the "step" for number inputs and sliders. So in this case, each notch on the slider or increment/decrement of the number input jumps by 8.

Ok - that's the field as defined in the node, without any user-configured constraints. We'll call these the inherited values.

If the user overrides the min and max, the override values are constrained first by each other and then by inherited values if no overrides are set. It is not possible to set the overrides in such a way that they are any less strict than the inherited values - they can only be as strict or stricter.

I started writing a specific numerical example, but it's way easier to just show how it works:

Screen.Recording.2025-03-14.at.9.11.23.pm.mov

Caveats:

  • There is no override for multipleOf (aka step). It's possible to add it but I'd had enough of this math-y stuff for the day and didn't get to it. Can revisit if needed.

  • The min and max overrides are inclusive. Exclusive min and max are tricky to represent on a slider. Can revisit if needed.

  • The override values are not applied to the node field directly. In edit mode, it is possible to set the values to something outside the range of the overrides by changing the value directly on the node. For example, if the max width override was set to 1024 in the form, it's possible to click edit and select a higher value in the node directly.

    I think the node fields should stay 1-to-1 mapped to the python nodes and the form constraints should not be applied to the nodes.

    If somebody edits the node field directly and sets a value outside the form builder's overrides, there is no error indicator. It's possible to add it but it's an decent extra layer of complexity, and I am not convinced this edge case is worth the time. Can revisit if needed.

Related Issues / Discussions

Many convos on discord and offline

QA Instructions

Try it out. Try to break it from the form builder UI.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions bot added the frontend PRs that change frontend files label Mar 14, 2025
Copy link
Member

@hipsterusername hipsterusername left a comment

Choose a reason for hiding this comment

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

works well from my initial testing.

@psychedelicious psychedelicious enabled auto-merge (rebase) March 16, 2025 23:44
@psychedelicious psychedelicious force-pushed the psyche/feat/ui/form-field-configuration branch from 7196203 to ea36799 Compare March 16, 2025 23:44
@psychedelicious psychedelicious merged commit d65ec0e into main Mar 16, 2025
15 checks passed
@psychedelicious psychedelicious deleted the psyche/feat/ui/form-field-configuration branch March 16, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants