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

Refactor ClampedVariable into Variable for numeric types? #62

Closed
jeffcampbellmakesgames opened this issue Apr 18, 2019 · 4 comments
Closed
Labels
question Further information is requested
Milestone

Comments

@jeffcampbellmakesgames
Copy link
Contributor

jeffcampbellmakesgames commented Apr 18, 2019

Looking through the issues for ClampedVariable, it wasn't clear to me why these are separate types from their normal Variable versions. It does seem like:

  • The small additional functionality and fields ClampedVariables have in comparison to Variable could be refactored into the Variable types fairly easily.
  • The public virtual member ClampValue overriden by each type seems to be only used internally in SetValue, meaning there would be no specific API lost by making this change. ClampValue could be implemented as private or internal on each of the comparable Variable types with the current implementation and would not require a new virtual or abstract member on BaseVariable that would clutter up non-comparable types.
  • There is already the concept of Variable potentially being either immutable or mutable by marking it as ReadOnly (no separate constant Variable type definition), so modifying it when mutable to allow for clamping would be consistent with current functionality.

The benefits of making this change would be:

  • No separate variable types for clamped vs unclamped. This would make it easier for a user to be able to change the intent of a variable to be either clamped or not in the editor without needing to create a separate asset and swapping its references from the old asset to do so.
  • Leaner codebase: separate runtime/editor/code generation code for clamped variables could be immediately removed in their entirety or marked as deprecated to allow users to swap their usage of these assets to normal variables over time and then.

The downside would be:

  • Existing property drawers would need to be modified to take into account that a variable might be clampable versus not. I have a few additions I'd like to make anyways for clamped values in the inspector like better validation of the current value w.r.t its min/max, possibly changing the value int field to a slider if clamped, etc... so I would probably take this on at the same time.
  • It would be beneficial to phase out clamped variables code/content over time to not immediately cause issues for end-users who have been using them in their projects already, but if this were to be done immediately it would break those existing assets for end-users.

Is there any background that could be given on why these were implemented as separate types and not additional functionality for comparable types? I may be missing a design intent here.

@DanielEverland DanielEverland added this to the Release 1.6.0 milestone Apr 18, 2019
@DanielEverland DanielEverland added the question Further information is requested label Apr 18, 2019
@jeffcampbellmakesgames
Copy link
Contributor Author

Just touching back on this to see if you have any thoughts @DanielEverland?

@DanielEverland
Copy link
Owner

I'll have time to check this out tomorrow

@DanielEverland
Copy link
Owner

This sounds like a good idea, I'll get on it right away.

@Jeffrey-Campbell-Zapdot
Copy link

If there is anything I can do to help just let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants