Re: Constraining properties

[Tim Janik <timj gnu org>]

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 <stefan space twc de>
> | 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 <stefan space twc de>
> | 
>
> (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 <stefan space twc de>
> | 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 <stefan space twc de>
> | 
>
> (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 <stefan space twc de>
> | Date:   Sun Apr 28 16:39:22 2019 +0200
> | 
> |     BSE: bsesong: use APPLY_IDL_PROPERTY for loop_enabled
> |     
> |     Signed-off-by: Stefan Westerfeld <stefan space twc de>
> | 
>
> (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.