From our last BseSong merge:
I think it might be better to re-add range enforcement. Right now, I would have to write this setter code:
Note that we duplicate the defaults here, which is not ideal. So we could also check against the pspec, somewhat like
But I think ultimately what I really want to write is this code:
and have the actual denominator (int val) function be generated automatically, which should
Consider this a brianstorming idea I at least wanted to share. Maybe the actual implementation will be a bit different, but I think the general idea to have each setter always execute some steps at the beginning and some steps at the end of setting a property is likely to make our actual code simpler. — _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
Hey Stefan,
since you're brain storming here and started a discussion, I'm taking this to the list and off the bug tracker. There are a few reasons I have not attempted to add enforcement with the new IDL API. First, the range information the IDL layer has is informal and mostly passed from *.idl to the __aida_aux_data__() method as opaque string. Second, even if the IDL layer was extended to parse the information and we'd start to formally specify what information has and has not to be present per property, it's not clear to me at what layer we'd generate automatic enforcement. Note that it gets even more tricky in the actual implementation, see my inline remarks below. On 31.08.2018 21:33, Stefan Westerfeld wrote:
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. And whether a property needs undo recording (e.g. a bpm change) or no undo recording (like a play position or loop pointer), is also entirely up to the implementation. Furthermore, whether a value can currently be changed (like the instrument setup used on a track) may depend on whether a project is currently in playback mode or not (that's the BSE_SOURCE_PREPARED flag in libbse), so entirely up to the implementation.
There's no "pspec" in the new API, while we still generate compat pspecs for the old beast-gtk code from the IDL aux info, there's no such thing in ebeast. I.e. checks can only be performed against __aida_aux_data__ strings. And without special knowledge about properties, etc, those look just like a bunch of foreign gibberish characters.
As I said, above, we don't have that anymore.
That would break the SynDrum trigger button.
That highly depends on the property and I'd rather not push that into the IDL layer as well.
Notification is implemented at the Bse::Object layer, not the IDL layer below that, so that'd be tricky also.
Yeah, I appreciate that in principle, but I hope I have succeeded in outlining why I thought there's no trivial and obvious way to implement this. I hear what you mainly want to simplify though, and I think a good chunk of that can be achieved with convenience macros, even if that's less elegant to the trained C++ eye. Assuming we move properties in to the *Impl classes and out of the C structs at some point, this could look like: void SongImpl::bpm (int val) { const bool changed = UPDATE_UNDO_PROPERTY (bpm_, val); if (changed) this->trigger_other_updates(); } With the following impl details, UPDATE_UNDO_PROPERTY(member_,value) would: 1) compare this->bpm_ != value 2) extract aux_info strings for "bpm", to perforrm range constraints 2) push the old value to undo (there could be an UPDATE_PROPERTY variant without undo) 3) notify("bpm"), the string "bpm" is probably best inferred from __func__, since that's the one IDL generated piece of information that UPDATE_PROPERTY has access to which perfectly matches the property name. We would need a couple test cases to assert that the runtime platform really assigns the property setter name to __func__ correctly, though. But afaik, that should be true for g++ (linux, windows) and clang (linux, windows, macos). As long as we keep the C structs around, we probably have to live with something like: BseSong *self = as<BseSong*>(); UPDATE_UNDO_C_PROPERTY(&self->bpm, val); Note that meta info about "bpm" can still be extracted and notifications for "bpm" can still be sent out by this macro, if we infer the property name from __func__.
-- Yours sincerely, Tim Janik --- https://testbit.eu/timj/ Free software author. _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
In reply to this post by Gnome - Beast mailing list
Great input Stefan. But please take brainstorming and discussions to the mailing list: https://mail.gnome.org/archives/beast/2018-August/msg00101.html — _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
In reply to this post by Gnome - Beast mailing list
Closed #74. — _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
In reply to this post by Gnome - Beast mailing list
Hi!
On Fri, Aug 31, 2018 at 11:53:07PM +0200, Tim Janik via beast wrote: > since you're brain storming here and started a discussion, I'm taking this to > the list and off the bug tracker. I won't comment on each thing you stated, yes, there were issues with my suggestion. Basically - you want a lightweight solution - you want to avoid code generation if possible - you want it to be optional and not force the same steps for every setter - there were minor technical issues to by suggestion, but these could be fixed while keeping the proposal > Now, whether the impl CLAMPs or ignores the setting is entirely up to the > implementation. All right, I'll keep that in mind. > And whether a property needs undo recording (e.g. a bpm change) or no undo > recording > (like a play position or loop pointer), is also entirely up to the > implementation. Right. We should also mention here that properties that do need undo recording need a way of suppressing undo. In the old codebase you use g_object_set if you don't want undo while setting a property. Also see "Most Bus properties ported to C++ #78". > I hear what you mainly want to simplify though, and I think a good chunk of > that can be achieved with convenience macros, even if that's less elegant to > the trained C++ eye. > Assuming we move properties in to the *Impl classes and out of the C structs > at some point, this could look like: > > > void > SongImpl::bpm (int val) > { > const bool changed = UPDATE_UNDO_PROPERTY (bpm_, val); > if (changed) > this->trigger_other_updates(); > } > > With the following impl details, UPDATE_UNDO_PROPERTY(member_,value) would: > > 1) compare this->bpm_ != value > 2) extract aux_info strings for "bpm", to perforrm range constraints > 2) push the old value to undo (there could be an UPDATE_PROPERTY variant > without undo) > 3) notify("bpm"), the string "bpm" is probably best inferred from __func__, > since that's the > one IDL generated piece of information that UPDATE_PROPERTY has access to > which > perfectly matches the property name. > We would need a couple test cases to assert that the runtime platform > really assigns the property setter name to __func__ correctly, though. But > afaik, that > should be true for g++ (linux, windows) and clang (linux, windows, macos). > > As long as we keep the C structs around, we probably have to live with > something like: > > BseSong *self = as<BseSong*>(); > UPDATE_UNDO_C_PROPERTY(&self->bpm, val); > > Note that meta info about "bpm" can still be extracted and notifications for > "bpm" > can still be sent out by this macro, if we infer the property name from > __func__. I think we should avoid using a macro here, if we can find an elegant C++ alternative which does what we need. Especially using the property setter __func__ to extract the property name and depend on compiler specific internals should't be done. This is too magic for my taste. On the other hand I think passing the actual variable is too simplistic, as often you want to do more than just an assignment. So I suggest with my previous example property to do it like this: void SongImpl::denominator (int val) { BseSong *self = as<BseSong*>(); update ("denominator", val, [&]() { self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1); bse_song_update_tpsi_SL (self); }); } and implement a generic: void update (const char *what, int& val, std::function<void()> func); which (1) checks if get ("denominator") != value (aida has generic getters) - if this is true (2) performs range checks and _modifies_ val if necessary (3) pushes undo (4) notifies "denominator" not using a macro here also allows us to pass a pointer to the variable which stores the value as an overloaded version, i.e. update ("bpm", val, &bpm_); Finally, undo: as far as I can see, the old way of doing this is defining SKIP_UNDO in the idl file, so that the property can query whether undo information should be recorded or not. So if I understand you correctly you want me to remove this information from the .idl file, and instead do it in the implementatation only. So we can simply use the function name to encode this feature. So we could use update_undo ("bpm", val, &bpm_); if we need undo, and the C++ file would be the only place where we can see if a property is recording undo information or not. With clamp I'm not sure whether we should force range enforcement always to be done automatically if you use update() at all, or if not. But we could add two more function, namely update_clamp ("bpm", val, &bpm_); update_clamp_undo ("bpm", val, &bpm_); But if we add this, it should cover all cases. As for whether to parse the range for the range check step (2) from the strings or not, it seems to be a matter of taste. You currently already generate __aida_get__ so I see no reason not to generate bool SongIface::__aida_range__ (const std::string &__n_, Aida::Any& __min_, Aida::Any& __max_) const { ... if (__n_ == "bpm") { __min_.set (1); __max_.set (1024); return true; } ... } Yes, its tricky because you need to know which string is min and which string is max, but on the other hand, if you want to parse the generated strings, you also need that information in order to perform the range check. Or be radical and always force implementors to write even in simple cases: update ("bpm", val, [&]() { val = CLAMP (val, 1, 1024); bpm_ = val; }); Cu... Stefan -- Stefan Westerfeld, http://space.twc.de/~stefan _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
On 14.09.2018 15:28, Stefan Westerfeld wrote:
>> I hear what you mainly want to simplify though, and I think a good chunk of >> that can be achieved with convenience macros, even if that's less elegant to >> the trained C++ eye. >> Assuming we move properties in to the *Impl classes and out of the C structs >> at some point, this could look like: >> >> >> void >> SongImpl::bpm (int val) >> { >> const bool changed = UPDATE_UNDO_PROPERTY (bpm_, val); >> if (changed) >> this->trigger_other_updates(); >> } >> >> With the following impl details, UPDATE_UNDO_PROPERTY(member_,value) would: >> >> 1) compare this->bpm_ != value >> 2) extract aux_info strings for "bpm", to perforrm range constraints >> 2) push the old value to undo (there could be an UPDATE_PROPERTY variant >> without undo) >> 3) notify("bpm"), the string "bpm" is probably best inferred from __func__, >> since that's the >> one IDL generated piece of information that UPDATE_PROPERTY has access to >> which >> perfectly matches the property name. >> We would need a couple test cases to assert that the runtime platform >> really assigns the property setter name to __func__ correctly, though. But >> afaik, that >> should be true for g++ (linux, windows) and clang (linux, windows, macos). >> >> As long as we keep the C structs around, we probably have to live with >> something like: >> >> BseSong *self = as<BseSong*>(); >> UPDATE_UNDO_C_PROPERTY(&self->bpm, val); >> >> Note that meta info about "bpm" can still be extracted and notifications for >> "bpm" >> can still be sent out by this macro, if we infer the property name from >> __func__. > I think we should avoid using a macro here, if we can find an elegant C++ > alternative which does what we need. Especially using the property setter > __func__ to extract the property name and depend on compiler specific internals > should't be done. This is too magic for my taste. That's ok, we know the compilers we're supporting, clang and gcc and __func__ works perfectly for both of them (it's part of the C++ standard actually). The reason I suggested using __func__ here is that this takes one major possibility for typos / human error away. If we don't automate the notification step, I doubt the whole effort is worth the hassle of implementing a "generic" setter mechanism. > On the other hand I think passing the actual variable is too simplistic, as > often you want to do more than just an assignment. So I suggest with my > previous example property to do it like this: > > void > SongImpl::denominator (int val) > { > BseSong *self = as<BseSong*>(); > > update ("denominator", val, [&]() { > self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1); > bse_song_update_tpsi_SL (self); > }); > } > As I see it, that hardly saves us anything. We basically remove three lines: - return_unless (val != bpm_); - push_property_undo ("bpm"); - notify ("bpm"); But add two new ones for a nested lambda construct (which is fairly complicated in comparison). I.e. saving 1 out of 3 lines isn't worth the mental overhead and labour involved for adding a mechanism. Again, if this was actually *helping* with making things less error prone, I'd be more inclined to consider it, but that got lost with still requiring the user to duplicate the property name as string. > With clamp I'm not sure whether we should force range enforcement always to be > done automatically if you use update() at all, or if not. But we could add two > more function, namely > > update_clamp ("bpm", val, &bpm_); > update_clamp_undo ("bpm", val, &bpm_); Nah, that's useful only for a fraction of the properties we expose. More below. > As for whether to parse the range for the range check step (2) from the strings > or not, it seems to be a matter of taste. You currently already generate > __aida_get__ so I see no reason not to generate > > bool > SongIface::__aida_range__ (const std::string &__n_, Aida::Any& __min_, Aida::Any& __max_) const > { > ... > if (__n_ == "bpm") { __min_.set (1); __max_.set (1024); return true; } > ... > } Here is the reason: clamping the property value into a range is only useful for a fraction of the (numeric) properties that are being used. The IDL file mainly covers what you'd usually have in a C++ file as well, the storage type (int, float, string), the name and the enclosing class. Optionally, *arbitrary* meta data can be "attached" to a property, it's best to think of this as a key=value list of strings (because it is), termed __aida_aux_data__ in aida.hh and provided as a string vector. The IDL file allows specification of these auxillary key=value string lists, the C++ binding implementation provides these via an accessor (including inheritance from base classes), the C++ client API exposes these, as does the Javascript client API. That way, we can add *arbitrary* auxillary data in the future, without having to adapt the middleware and all programming language bindings. E.g. we can add minimum int/float, maximum int/float, stepping int/float or paging int/float values. Also we can add valid/invalid charsets for strings (e.g. to constrain date inputs), add flags like STORAGE, GUI_ONLY, DEPRECATED for all sorts of properties. We can add unit hints (Hz, %, Cents), add a base for logarithmic scales, etc, etc. All of these annotations can be added by the IDL authors and optionally be supported by UI front ends or storage backends, *without* middle layer adaptions. The __aida_range__ proposal does away with that benefit. And it poses a question about how many other property kinds warrant extra API. And it causes more language binding implementation effort, it does that *despite* the actual information already being available through the existing __aida_aux_data__ vector. There're two cases in the Beast code base that warrant an extension of the property accessor API beyond __aida_get__ + __aida_set__, those are: 1) Exposing whether a property is *temporarily* read-only (e.g. during playback); 2) Listing candidates that can be used in the setter, this only applies to objects though (enums are covered by a static enum introspection API). Both are useful for *dynamic* property behaviour, i.e. the candidate list can change during runtime, as can the temporary read-only state. At the moment, our old and new APIs ignore (1) and (2) can be implemented at the BseObject level without needing middleware support. > Yes, its tricky because you need to know which string is min and which string is > max, but on the other hand, if you want to parse the generated strings, you also > need that information in order to perform the range check. > > Or be radical and always force implementors to write even in simple cases: > > update ("bpm", val, [&]() { val = CLAMP (val, 1, 1024); bpm_ = val; }); We have maybe 100 or 200 properties, any significant middleware extensions should simplify or significantly improve the *majority* of accessor implementations to warrant the efforts involved. If not, we're better off using our time for just porting the remaining cases. As an aside, we have not yet considered automation. Looking at DAWs out there, we probably want to make *all* float + int properties automatable in the future anyway (maybe even enums like filter/oscillator types). And that could potentially change the int/float/enum accessors a whole lot yet again. Input on that front is welcome also, but should go into a separate thread. > 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 |
Hi!
On Fri, Sep 14, 2018 at 05:59:43PM +0200, Tim Janik wrote: > On 14.09.2018 15:28, Stefan Westerfeld wrote: > >> I hear what you mainly want to simplify though, and I think a good chunk of > >> that can be achieved with convenience macros, even if that's less elegant to > >> the trained C++ eye. > >> Assuming we move properties in to the *Impl classes and out of the C structs > >> at some point, this could look like: > >> > >> > >> void > >> SongImpl::bpm (int val) > >> { > >> const bool changed = UPDATE_UNDO_PROPERTY (bpm_, val); > >> if (changed) > >> this->trigger_other_updates(); > >> } > >> > >> With the following impl details, UPDATE_UNDO_PROPERTY(member_,value) would: > >> > >> 1) compare this->bpm_ != value > >> 2) extract aux_info strings for "bpm", to perforrm range constraints An important step in the context of this discussion. > >> 2) push the old value to undo (there could be an UPDATE_PROPERTY variant > >> without undo) > >> 3) notify("bpm"), the string "bpm" is probably best inferred from __func__, > >> since that's the > >> one IDL generated piece of information that UPDATE_PROPERTY has access to > >> which > >> perfectly matches the property name. > >> We would need a couple test cases to assert that the runtime platform > >> really assigns the property setter name to __func__ correctly, though. But > >> afaik, that > >> should be true for g++ (linux, windows) and clang (linux, windows, macos). > >> > >> As long as we keep the C structs around, we probably have to live with > >> something like: > >> > >> BseSong *self = as<BseSong*>(); > >> UPDATE_UNDO_C_PROPERTY(&self->bpm, val); > >> > >> Note that meta info about "bpm" can still be extracted and notifications for > >> "bpm" > >> can still be sent out by this macro, if we infer the property name from > >> __func__. > > I think we should avoid using a macro here, if we can find an elegant C++ > > alternative which does what we need. Especially using the property setter > > __func__ to extract the property name and depend on compiler specific internals > > should't be done. This is too magic for my taste. > > That's ok, we know the compilers we're supporting, clang and gcc and > __func__ works perfectly for both of them (it's part of the C++ standard > actually). Ok. Nowerdays it is even supported by msvc compiler, so all right, lets use it, I can't think of a case where it would break compilation. > The reason I suggested using __func__ here is that this takes one > major possibility for typos / human error away. > If we don't automate the notification step, I doubt the whole effort is > worth the hassle of implementing a "generic" setter mechanism. Ok. There are two ways actually to achieve this: one would be extending the generic setter to use __func__. That will have to use macro, but I'll suggest two versions below. The other is different: avoid passing strings in the API. Often there are multiple properties that need to be notified, for instance the setter part of BusImpl::volume_db: auto prop = "volume_db"; push_property_undo (prop); [...] notify ("left_volume"); notify ("right_volume"); notify ("pan"); notify (prop); Even if the generic setter would eliminate prop, but there are three strings that the compiler cannot check. If we would write notify (p_left_volume); notify (p_right_volume); notify (p_pan); for the remaining notifications, we would at least catch typos here. And p_something would be an automatically generated static data member of the base class, in the easiest case a string, later if we have ported everything to C++, we could make this a Property class, and enforce that notify only gets called with a property class. > > On the other hand I think passing the actual variable is too simplistic, as > > often you want to do more than just an assignment. So I suggest with my > > previous example property to do it like this: > > > > void > > SongImpl::denominator (int val) > > { > > BseSong *self = as<BseSong*>(); > > > > update ("denominator", val, [&]() { > > self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1); > > bse_song_update_tpsi_SL (self); > > }); > > } > > > > As I see it, that hardly saves us anything. > We basically remove three lines: > > - return_unless (val != bpm_); > - push_property_undo ("bpm"); > - notify ("bpm"); Right. And clamping. I'll address this below. > But add two new ones for a nested lambda construct (which is fairly > complicated in comparison). I.e. saving 1 out of 3 lines isn't worth > the mental overhead and labour involved for adding a mechanism. > Again, if this was actually *helping* with making things less error > prone, I'd be more inclined to consider it, but that got lost with still > requiring the user to duplicate the property name as string. All right, if we have to get rid of the "denominator" here, and writing p_denominator as mentioned above is also not good enough, here are two macro versions. I've tested this with a complete C++ file, but I'll just show the macro definition and usage here. The first case is close to my original suggestion, and the macro only replaces the invocation with some lambda thing that adds __func__ to the invocation of the real update function #define UPDATE [&, what__ = __func__](auto& val, auto l){ update (what__, val, l); } So this would look like this: void SongImpl::denominator (int val) { BseSong *self = as<BseSong*>(); UPDATE (val, [&]() { self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1); bse_song_update_tpsi_SL (self); }); } I like the lambda as a way of expressing that the body update code may or may not be executed. Also its easy to executed the notification after the body. Once the variables are stored in the C++ class, things will get simpler. Another way would be to make the macro basically act like an if-statement. This again expresses conditional execution. To do notification after the setter, we need a helper class that does this in the destructor, if the conditional execution took place, as #define IF_UPDATE(val__) auto uh__ = UpdateHelper (__func__, this, get (__func__) != val__); if (uh__.changed) void SongImpl::denominator (int val) { BseSong *self = as<BseSong*>(); IF_UPDATE (val) { self->denominator = val <= 2 ? val : 1 << g_bit_storage (val - 1); bse_song_update_tpsi_SL (self); } } I'm not sure if this is good practice, as the IF_UPDATE expands to something that is not a complete statements, but only the "if"-part. > > With clamp I'm not sure whether we should force range enforcement always to be > > done automatically if you use update() at all, or if not. But we could add two > > more function, namely > > > > update_clamp ("bpm", val, &bpm_); > > update_clamp_undo ("bpm", val, &bpm_); > > Nah, that's useful only for a fraction of the properties we expose. More below. > > > As for whether to parse the range for the range check step (2) from the strings > > or not, it seems to be a matter of taste. You currently already generate > > __aida_get__ so I see no reason not to generate > > > > bool > > SongIface::__aida_range__ (const std::string &__n_, Aida::Any& __min_, Aida::Any& __max_) const > > { > > ... > > if (__n_ == "bpm") { __min_.set (1); __max_.set (1024); return true; } > > ... > > } > > Here is the reason: clamping the property value into a range is only useful > for a fraction of the (numeric) properties that are being used. > > The IDL file mainly covers what you'd usually have in a C++ file as well, the > storage type (int, float, string), the name and the enclosing class. > Optionally, *arbitrary* meta data can be "attached" to a property, it's best > to think of this as a key=value list of strings (because it is), > termed __aida_aux_data__ in aida.hh and provided as a string vector. > > The IDL file allows specification of these auxillary key=value string lists, the > C++ binding implementation provides these via an accessor (including > inheritance from base classes), the C++ client API exposes these, as does > the Javascript client API. > > That way, we can add *arbitrary* auxillary data in the future, without having to > adapt the middleware and all programming language bindings. E.g. we can add > minimum int/float, maximum int/float, stepping int/float or paging int/float values. > Also we can add valid/invalid charsets for strings (e.g. to constrain date inputs), > add flags like STORAGE, GUI_ONLY, DEPRECATED for all sorts of properties. > We can add unit hints (Hz, %, Cents), add a base for logarithmic scales, etc, etc. > > All of these annotations can be added by the IDL authors and optionally be > supported by UI front ends or storage backends, *without* middle layer adaptions. > > The __aida_range__ proposal does away with that benefit. And it poses a question > about how many other property kinds warrant extra API. And it causes more > language binding implementation effort, it does that *despite* the actual > information already being available through the existing __aida_aux_data__ > vector. Ok, then maybe my proposal was too specific. You don't want it in the generated language binding, and that in itself is ok. However, I'll still say that we should have *something* like __aida_range__. For the JavaScript UI, you will have code to query the range of a property. For beast-gtk, as long as it lives, you will have code to query the range of a property. So I argue that for libbse (the backend, the core), we should have some way to query the range of a property. This can then be used in the generic setter code. Whether you parse the range information for a property from the key=value strings, cache the parsed value, or whatever else, I'd like to have (for numeric properties) a way to say val = range_check ("bpm", val); I say this because the alternative would be to duplicate all range information for numeric properties, and this doesn't feel right val = CLAMP (val, 1, 1024); > We have maybe 100 or 200 properties, any significant middleware extensions > should simplify or significantly improve the *majority* of accessor implementations > to warrant the efforts involved. If not, we're better off using our time for just > porting the remaining cases. Ok, as I said: accessing the range of a property is not an esoteric request, but it seems clear that both ebeast and beast-gtk need that, so it might as well be a useful information for the core itself. > As an aside, we have not yet considered automation. [...] Right. But right now I think property porting is my main priority, so it is better to focus on that for a while. Cu... Stefan -- Stefan Westerfeld, http://space.twc.de/~stefan _______________________________________________ beast mailing list [hidden email] https://mail.gnome.org/mailman/listinfo/beast |
Free forum by Nabble | Edit this page |