Ok, so let me comment on anything that doesn't have to do with the new
volume_db/pan properties first.
> 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.
Right. This is a really nasty problem, which is caused by loading/reading code
for float64 properties in aida. It turns out that even simple floating point
numbers like "1.03" are written out correctly, but restore breaks in
bsestorage.cc, because the scanner_parse_paren_rest does return this as "1. 3"
and this is restored as a value of 1.0. I've fixed this by using the existing
sfi_scanner_parse_real_num function to parse float64 values (see also commit
comments). This seems to work, tests pass again. Not sure if this is what you
think we should do to fix the problem, though.
> 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)
Ok. You'll notice that the magic factors are gone, and the clamping looks sane
(CLAMP (v, -96, 12)). In particular I don't try to enforce restrictive CLAMP()
boundaries for left_factor and right_factor, they are now an implementation
detail and should not be used by the UI.
> 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.
Fixed.
> 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.
Ok. Maybe I'll try it later.
> ```
> + for (auto& c : pname)
> ```
> pointers and references are written with *& attached to the variable, i.e.
> write this as "auto &c"
Ok.
> ```
> + 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.
Fixed.
> ```
> + 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.
Fixed. But code was deleted in a later commit.
> + return (self == master);
> ```
> Please remove extraneous parenthesis like these, they could hide an
> assignment.
Fixed.
> ```
> + 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.
Fixed.
> 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)
Right. Consider this fixed, as we no longer need sync, and I see absolutely no
point in trying to be too perfect here. We read left_volume and right_volume in
any order, and that is it. The UI should always use the volume_db/pan
properties. If the user used sync before saving, left_volume and right_volume
will be the same, and pan will be 0. If not, left_volume and right_volume will
be different, and pan will not be 0.
So after commenting on your remarks, let be briefly describe what I did to get
volume_db/pan properties. First, I looked at some variations on how to compute
factors like our left_volume/right_volume from a total volume_db and panning. A
good description of the possibilities is here:
http://www.cs.cmu.edu/~music/icm-online/readings/panlaws/index.html
I choose to implement constant power panning (-3dB), because:
- some popular DAWs use it as default (for instance Cubase)
- the mathematical definition is quite straightforward (we want constant power)
- from this follows that we have all functions we need to implement translation
backwards/forwards
However this seems to be one of the things that don't have a globally optimal
solution. For instance I tried Bitwig, expecting to see constant power panning,
but they appear to use -4.5dB pan law (also confirmed on Forums). Cubase afaik
has a project wide setting for -6dB, -4.5dB, -3dB, 0dB, constant power
(default).
Long story short: I implemented the volume_db/pan properties as a view on the
underlying left_volume/right_volume factors with constant power panning (-3dB).
Constant power panning should be a good solution, and it gives the user the
ability to control volume and panning individually. We can change the default
later on, if we choose to implement other panning strategies.
I adapted the beast-gtk UI. As we have no knob widget, I use a horizontal
slider to represent panning. I believe that the new UI is definitely an
improvement over the old from a usability point of view, even if it may take
some initial effort to understand which widget is doing what.
You'll also find one somewhat clumsy commit that allows setting a
Bus::master_output property with and without undo. The old framework had the
possibility to suppress undo via g_object_set. I have here implemented one
method with and one without undo. However a framework solution would probably
be better in the long run, such as
{
auto no_undo = bus->scoped_undo_blocker();
bus->master_output (true); // not undoable
}
The other case would be
{
bus->master_output (true); // undoable
}
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/tim-janik/beast/pull/78#issuecomment-421138602_______________________________________________
beast mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/beast
