Constraining properties

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Constraining properties

Tim Janik-6
Hi Stefan,

as requested, I've prototyped a basic macro that helps with property setting by:

* constraining the property value (for int, float, enum)
* testing for value changes
* pushes undo (untested)
* emitting notification events
* actually setting the constrained value

Usage looks like this:

void
SongImpl::musical_tuning (MusicalTuning tuning)
{
  if (!prepared())
    {
      BseSong *self = as<BseSong*>();
      if (APPLY_IDL_PROPERTY (self->musical_tuning, tuning))
        {
          SfiRing *ring;
          for (ring = self->parts; ring; ring = sfi_ring_walk (ring, self->parts))
            bse_part_set_semitone_table ((BsePart*) ring->data, bse_semitone_table_from_tuning (self->musical_tuning));
        }
    }
}

You can find the code here:
        https://github.com/tim-janik/beast/tree/wip/constrain-properties

Please review the changes the branch introduces and let me know if that
corresponds to the ideas you had about constraining properties.



--
Yours sincerely,
Tim Janik

https://testbit.eu/timj
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: Constraining properties

Gnome - Beast mailing list
   Hi!

On Sun, Apr 07, 2019 at 02:45:23AM +0200, Tim Janik wrote:

> as requested, I've prototyped a basic macro that helps with property setting by:
>
> * constraining the property value (for int, float, enum)
> * testing for value changes
> * pushes undo (untested)
> * emitting notification events
> * actually setting the constrained value
>
> Usage looks like this:
>
> void
> SongImpl::musical_tuning (MusicalTuning tuning)
> {
>   if (!prepared())
>     {
>       BseSong *self = as<BseSong*>();
>       if (APPLY_IDL_PROPERTY (self->musical_tuning, tuning))
>         {
>           SfiRing *ring;
>           for (ring = self->parts; ring; ring = sfi_ring_walk (ring, self->parts))
>             bse_part_set_semitone_table ((BsePart*) ring->data, bse_semitone_table_from_tuning (self->musical_tuning));
>         }
>     }
> }
>
> You can find the code here:
> https://github.com/tim-janik/beast/tree/wip/constrain-properties
>
> Please review the changes the branch introduces and let me know if that
> corresponds to the ideas you had about constraining properties.

Thanks for implementing this, this looks like the functionality I wanted to
have. I've committed my own fixes on top of that

  https://github.com/swesterfeld/beast/tree/constrain-properties-plus

So mainly I'll comment my changes here:

| commit 12b9c83d7fe9423ddcfecc4cad7ea9f937a0bfb7
| Author: Stefan Westerfeld <[hidden email]>
| Date:   Sun Apr 28 16:31:10 2019 +0200
|
|     BSE: bseitem: fix APPLY_IDL_PROPERTY() bugs for non primitive types
|    
|     - if property was changed => update it
|     - call push_property_undo just like for primitive types
|    
|     Signed-off-by: Stefan Westerfeld <[hidden email]>
|

(1) I think this is just an obvious correction that spells out what you wanted
to do for non-primitive types.

| commit 976efd46f5954b4f89d0c75c5e8ec4809fc87118
| Author: Stefan Westerfeld <[hidden email]>
| Date:   Sun Apr 28 16:37:57 2019 +0200
|
|     BSE: bseitem: check if undo is needed in APPLY_IDL_PROPERTY()
|    
|     If "skip-undo" hint for the property is set, don't call push_property_undo.
|    
|     Signed-off-by: Stefan Westerfeld <[hidden email]>
|

(2) Here we have the question of how to handle undo. I think we should at least
allow using the macro for the non undo case. Without this fix, undo is done
unconditionally. I saw two possible solutions: one would be an extra
APPLY_IDL_PROPERTY_NO_UNDO macro, which would make the skip-undo hint
meaningless, and make whether a property is undoable or not an implementation
detail (not part of the interface).

I didn't do it this way, because we may have properties in plugins that are
not undoable, where the skip-undo hint would be very helpful, so I thought
it would be more consistent to do it like that everywhere.

| commit 3e37306f5181b50da213693e7edc577de9f9dcc1 (HEAD -> constrain-properties-plus, origin/constrain-properties-plus)
| Author: Stefan Westerfeld <[hidden email]>
| Date:   Sun Apr 28 16:39:22 2019 +0200
|
|     BSE: bsesong: use APPLY_IDL_PROPERTY for loop_enabled
|    
|     Signed-off-by: Stefan Westerfeld <[hidden email]>
|

(3) This is more a suggestion of how to deal with locks: I'm not sure if I like
my own code here; the question is what happens if we have to take a lock before
setting a value. We could do (like in the commit).

  bool value = self->loop_enabled_SL;

  if (APPLY_IDL_PROPERTY (value, enabled))
    {
      BSE_SEQUENCER_LOCK ();
      self->loop_enabled_SL = value;
      BSE_SEQUENCER_UNLOCK ();
    }

effectively using a temp value to be able to lock before we write the new
value, or we could do

  BSE_SEQUENCER_LOCK ();
  APPLY_IDL_PROPERTY (self->loop_enabled_SL, enabled);
  BSE_SEQUENCER_UNLOCK ();

which looks more sane, but at the expense of taking the lock for a slightly
longer duration. I'm not sure if reading a variable without lock is even
safe (its done often in bsesong.cc); in principle there might be platforms
which could provide half-written values.

The last option here would be not to use the macro at all, and preserve
the manually written code which handles stuff just fine.

(4) As last comment (without code), I'd like to point out that your macro
doesn't live up to your own standards. You wrote

http://gtk.10911.n7.nabble.com/tim-janik-beast-Smarter-property-set-implementation-74-td94379.html#a94525

| The above actually needs to be changed to the following order:
|
|
|   if (int (self->denominator) != val)
|     {
|       val = CLAMP (val, 1, 256);
|       // ...
|       notify (prop);
|     }
|
| Here's why:
|
| a) A user enters -77 at the UI, which is reflected by a text entry or slider.
| b) The UI sends -77 through the IDL layer.
| c) The impl refuses -77 and CLAMPs it to 0, or maybe retains the old value, e.g. 10.
| d) Because the value the UI displays (-77) and the value that was sent (-77) is
|     different from the resulting value in the impl (e.g. 0 or 10), a change
| notification
|     *must* be sent back to the UI (and other monitoring instances), in order for the
|     UI to be notified that the value being displayed has to be updated by querying
|     the impl again.
|
| Now, whether the impl CLAMPs or ignores the setting is entirely up to the
| implementation.

So if I understand it correctly, if you enter -77 at the GUI, you want to send
a notification in every case. However, your implementation will return false
early for this case. Look at the FIXME below.

  /// Constrain and assign property value if it has changed, emit notification.
  template<class T> bool
  apply_idl_property (T &lvalue, const T &cvalue, const String &propname)
  {
    if (std::is_integral<T>::value || std::is_floating_point<T>::value || std::is_enum<T>::value)
      { // avoid value copy for non primitive types
        T rvalue = cvalue;
        StringVector kvlist = find_prop (propname);
        if (!constrain_idl_property (rvalue, kvlist))
          return false;
        if (lvalue == rvalue)
          return false; // FIXME
        if (apply_idl_property_need_undo (kvlist))
          push_property_undo (propname);
        lvalue = std::move (rvalue);
      }
    else
      {
        if (lvalue == cvalue)
          return false;
        if (apply_idl_property_need_undo (find_prop (propname)))
          push_property_undo (propname);
        lvalue = cvalue;
      }
    auto lifeguard = shared_from_this();
    exec_now ([this, propname, lifeguard] () { this->notify (propname); });
    return true;
  }

   Cu... Stefan
--
Stefan Westerfeld, http://space.twc.de/~stefan
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast
Reply | Threaded
Open this post in threaded view
|

Re: Constraining properties

Tim Janik-6

On 28.04.19 17:10, Stefan Westerfeld via beast wrote:
> On Sun, Apr 07, 2019 at 02:45:23AM +0200, Tim Janik wrote:
>> as requested, I've prototyped a basic macro that helps with property setting by:

>
> So mainly I'll comment my changes here:
>
> | commit 12b9c83d7fe9423ddcfecc4cad7ea9f937a0bfb7
> | Author: Stefan Westerfeld <[hidden email]>
> | Date:   Sun Apr 28 16:31:10 2019 +0200
> |
> |     BSE: bseitem: fix APPLY_IDL_PROPERTY() bugs for non primitive types
> |    
> |     - if property was changed => update it
> |     - call push_property_undo just like for primitive types
> |    
> |     Signed-off-by: Stefan Westerfeld <[hidden email]>
> |
>
> (1) I think this is just an obvious correction that spells out what you wanted
> to do for non-primitive types.

Not really. How did you test this for non-primitive types?
Actually the code I prototyped is wrong, if you take a close look,
it does a std::move for primitives and a dead-code (!!) copy assignment for
non-primitive types. Nobody needs to std::move a double.

I'm thinking about to just have the code error out for non-primitive types,
that's simply not what it's intended for and I don't know a test case off head.

> | commit 976efd46f5954b4f89d0c75c5e8ec4809fc87118
> | Author: Stefan Westerfeld <[hidden email]>
> | Date:   Sun Apr 28 16:37:57 2019 +0200
> |
> |     BSE: bseitem: check if undo is needed in APPLY_IDL_PROPERTY()
> |    
> |     If "skip-undo" hint for the property is set, don't call push_property_undo.
> |    
> |     Signed-off-by: Stefan Westerfeld <[hidden email]>
> |
>
> (2) Here we have the question of how to handle undo.> I think we should at least
> allow using the macro for the non undo case. Without this fix, undo is done
> unconditionally. I saw two possible solutions: one would be an extra
> APPLY_IDL_PROPERTY_NO_UNDO macro, which would make the skip-undo hint
> meaningless, and make whether a property is undoable or not an implementation
> detail (not part of the interface).
>
> I didn't do it this way, because we may have properties in plugins that are
> not undoable, where the skip-undo hint would be very helpful, so I thought
> it would be more consistent to do it like that everywhere.

Can you elaborate on this? How's a plugin different here from a module or object
implemented inside bse/ ?

> | commit 3e37306f5181b50da213693e7edc577de9f9dcc1 (HEAD -> constrain-properties-plus, origin/constrain-properties-plus)
> | Author: Stefan Westerfeld <[hidden email]>
> | Date:   Sun Apr 28 16:39:22 2019 +0200
> |
> |     BSE: bsesong: use APPLY_IDL_PROPERTY for loop_enabled
> |    
> |     Signed-off-by: Stefan Westerfeld <[hidden email]>
> |
>
> (3) This is more a suggestion of how to deal with locks: I'm not sure if I like
> my own code here; the question is what happens if we have to take a lock before
> setting a value. We could do (like in the commit).
>
>   bool value = self->loop_enabled_SL;
>
>   if (APPLY_IDL_PROPERTY (value, enabled))
>     {
>       BSE_SEQUENCER_LOCK ();
>       self->loop_enabled_SL = value;
>       BSE_SEQUENCER_UNLOCK ();
>     }
>
> effectively using a temp value to be able to lock before we write the new
> value,

In general, a lock has to be held as short as possible, so this is the only
sensible implementation.

> or we could do
>
>   BSE_SEQUENCER_LOCK ();
>   APPLY_IDL_PROPERTY (self->loop_enabled_SL, enabled);
>   BSE_SEQUENCER_UNLOCK ();

No, apply_idl_property employs significantly complex and memory intensive
logic, it even calls other library functions that need to acquire locks as
well (exec_now), so this risks holding the lock too long and theoretically
deadlocks (and *proving* there will never be a deadlock possible when two or
more mutexes are involved is *very* hard, given that our code base and GLib's
are constantly moving and are littered with callbacks).

As an aside, this sequencer code falls into the "Blocked by Synth Engine"
category and will be rewritten at some point to avoid BSE_SEQUENCER_LOCK.

> which looks more sane, but at the expense of taking the lock for a slightly
> longer duration. I'm not sure if reading a variable without lock is even
> safe (its done often in bsesong.cc); in principle there might be platforms
> which could provide half-written values.

Half-written ints or doubles are generally not a problem, but code motion is, so
a lock, an atomic instruction or at least careful barriers are always needed to
synchronize updates between concurrent accesses.

> The last option here would be not to use the macro at all, and preserve
> the manually written code which handles stuff just fine.
>
> (4) As last comment (without code), I'd like to point out that your macro
> doesn't live up to your own standards.

This might come as a surprise for you, but that's what 'prototyping' is about,
as I wrote in the email you just quoted. ;-)

> You wrote
>
> http://gtk.10911.n7.nabble.com/tim-janik-beast-Smarter-property-set-implementation-74-td94379.html#a94525
>
> | The above actually needs to be changed to the following order:
> |
> |
> |   if (int (self->denominator) != val)
> |     {
> |       val = CLAMP (val, 1, 256);
> |       // ...
> |       notify (prop);
> |     }
> |
> | Here's why:
> |
> | a) A user enters -77 at the UI, which is reflected by a text entry or slider.
> | b) The UI sends -77 through the IDL layer.
> | c) The impl refuses -77 and CLAMPs it to 0, or maybe retains the old value, e.g. 10.
> | d) Because the value the UI displays (-77) and the value that was sent (-77) is
> |     different from the resulting value in the impl (e.g. 0 or 10), a change
> | notification
> |     *must* be sent back to the UI (and other monitoring instances), in order for the
> |     UI to be notified that the value being displayed has to be updated by querying
> |     the impl again.
> |
> | Now, whether the impl CLAMPs or ignores the setting is entirely up to the
> | implementation.
>
> So if I understand it correctly, if you enter -77 at the GUI, you want to send
> a notification in every case. However, your implementation will return false
> early for this case.

Yes, good catch indeed.

> Look at the FIXME below.
[...]
>         if (lvalue == rvalue)
>           return false; // FIXME

I have a locally rebased wip/constrain-properties branch with your changes on
top. But I'm afraid I'll throw those away (and a bit of my code too) to fix this
issue and rule out non-primitive types.

Thanks for the review, once ready I'll push the macro with the fixes on top,
that should give you all you need to continue on the property ports.

>    Cu... Stefan
>

--
Yours sincerely,
Tim Janik

https://testbit.eu/timj
Free software author.
_______________________________________________
beast mailing list
[hidden email]
https://mail.gnome.org/mailman/listinfo/beast