Thanks Stefan, great to see steady progress here.
But note that travis-ci shows, that your changes broke the syndrum audio test
in all build variants, please take a look at that.
About volume clamping, taking a look at other DAWs suggests that this should be
done at the UI and doesn't need to be based on consts from the IDL at all.
Also the property descriptios read "Volume adjustment in decibel of left/right
bus channel" and that's what we should make them. I.e. we should pass actual dB
values, e.g. 96 as property value through the IDL layer. (*1)
About GParamSpec containing minus, that's because GLib enforces '-' instead of
'_' in the pspec names, so we're forced to work around that in other places as
well. Please take a look at name_to_identifier in bstparam.cc, seems like it's
time to move that to bstutils.hh to avoid gratituous duplicatoin everywhere.
About porting object / sequence properties, I'm actually not sure what's
missing there atm. If you want to make an attempt at porting such a property
and nag me about missing bits in the IDL layer or similar, that'd be ok as well.
+ for (auto& c : pname)
pointers and references are written with *& attached to the variable, i.e.
write this as "auto &c"
+ Const BUS_VOLUME_MIN = 1.58489319246111e-05; /* -96dB */
if at all, this should be using '96' as Const value, and the const be defined
in terms of dB. but in other DAWs, the dB range displayed is a function of the
UI, e.g. the volume meter of a track display my use different limits than for a
related bus channel volume meter.
+ self->right_volume = self->left_volume = center_volume
(self->right_volume, self->left_volume);
While you're at it, please make that a two liner.
Modern code analysis tools tend to choke on using an assignment as rvalue, and
its still very readable if it's just:
self->left_volume = center_volume (self->right_volume,
self->left_volume);
self->right_volume = self->left_volume;
+ return (self == master);
Please remove extraneous parenthesis like these, they could hide an assignment.
+ auto parent = BSE_ITEM (self)->parent;
Please don't use 'auto' here, but BseItem*, like you did in other places. The
reason here is that the variable is the result of a hidden C-style cast
(BSE_ITEM()) and used as input for another C-style cast (BSE_SONG), so the
actual type used here is completely unclear to the reader and potentially
hiding a type bug.
On a side note, it took a while, but I figured again the point of saved_sync.
It's clearly trying to preserve the 'sync' state around loading a BSE file
(termed 'restore' in the API), and sets that initially to FALSE to allow BSE
files to restore different left and right volumes, and syncs thouse only if
"sync" is also set or after the restore phase is completed. That heavily
depends on the order in which the properties are defined in the BSE file which
is somewhat fragile... (*2)
I get the feeling that the old API is a bit too low level with using synced
volume factors instead of actual dB values. (*1) and (*2) are both indications
that we should probably move towards something like:
* Factors used internally: left_volume, right_volume - though a bit of compat
code will be needed to still load these values from old BSE files; syncing is
implemented here (as before)
* New API: left_volume_db, right_volume_db - values passed in actual dB, these
values can still be different when read even if sync is enabled.
* So essentially, sync, left_volume_db, right_volume_db can all be read/set
independently of each other and the internal bus logic adjusts left_volume,
right_volume accordingly.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tim-janik/beast/pull/78#issuecomment-419684838_______________________________________________
beast mailing list
beast@gnome.org
https://mail.gnome.org/mailman/listinfo/beast
