-
Notifications
You must be signed in to change notification settings - Fork 18
Add MultiValueArithmeticDriver<T> Component #146
Conversation
Not sure why the check is failing :/ |
OH yeah the github action script has been broken for a long time |
has this been fully tested in a multi user enviroment |
No not yet. |
when do you want to test ? |
sorry I have been busy with stuff so haven't been able to test, but hopefully soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR looks alright, better than most other people's code I've had to read. There's a couple small changes here and there that I'd like to see
using System.Linq; | ||
using BaseX; | ||
using FrooxEngine; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a namespace defined here,
namespace FrooxEngine;
should do.
(Linka please fix the namespaces in your code it's missing in a couple of places and dropping stuff to the global namespace is bad)
|
||
protected override void OnChanges() | ||
{ | ||
if (!Target.IsLinkValid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could probably be reformatted to something like this
if (!Target.IsLinkValid || Values.Count == 0) return;
} | ||
if (Values.Contains(Target.Target)) | ||
{ | ||
// don't let the component drive itself, don't want a feedback loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
approved |
It's basically a small calculator in components. You can add, subtract, multiply or divide with it. You can also type values straight into the component or drive them from elsewhere.