Hi Tim, Am 15.01.2014 03:20, schrieb Tim E. Real: > I've been examining changes in the master. > I'm not sure that some of them should have been added to master > without further pondering. Grab a coffee... > > 1) Elimination of shared event lists for clone parts: > ------------------------------------------------------- > Yeah, I might have endorsed this and it may have seemed like a > good idea at the time. But now I fear it was a mistake. > We have traded the simplicity, convenience, and the guaranteed identity > of shared events for a complicated and slow process of searching > other clone parts' event lists for 'equal' events so that they may > be altered.
Nope, we haven't. Not only:
We have traded the uglyness and complexity or porting CTRL value changes
and the like for an equally complicated process of searching...
I mean, it is true that replicating a change to all clones is ugly BUT
clones forced us to ugly things before, which now became obsolete. We
traded crap by crap :-|
But there's a light in the dark: We also traded this for a new
possibility, waiting to be implemented:
Patterns, or template parts!
These are "parametrized clones", for example they hold notes of an
arpeggiating piano. But you can actually set the chords! (By setting
chords, and not by altering each single note).
You could basically tell MusE:
"an arpeggio in C is C E G C' E G :||
now play arpeggio in | C | F | G G7 | C |"
with no effort at all :)
>
> Guaranteeing that two clone event lists are truly identical is going to be
> difficult, but possible. I think it is currently failing to do so.
That's a bug then, but not a design flaw. Additionally, we should give
each event a UUID, which is different for different events, but which is
the same for events over clone parts.
> First, please do *not* use dynamic_cast unless absolutely necessary,
> and try to find workarounds instead.
> dynamic_cast is slow, and brings in RTTI support.
slowness is not an argument here. I mean, come on, dynamic_cast is DAMN
fast on a recent machine. And what we're doing is basically re-invent
the RTTI-Wheel with our Event::type(). But ok, we could change that
here, no problem, go ahead.
> If we can eliminate all dynamic_cast then we don't need RTTI support.
> Yes, it's a magic feature and one I used to use a lot too.
> But no more. Instead I use 'type' members in classes so that simple
> casts can be used. You'll notice I've been slowly adding these 'type'
> members over the years and trying to *eliminate* all dynamic_cast.
Why reinvent the wheel when C++ already gives us what we need?
> Second, the use of PosLen::operator== in the second section above is
> *not* sufficient.
> That operator only compares the position, not the length.
> Thus it may select or modify the /wrong/ event from two events at the
> same position but with differing lengths. So your 'clone' event lists will
> no longer be identical.
>
> An obvious solution would be to override operator== in class PosLen,
> so that it compares BOTH position and length.
> I looked at that some time ago and IIRC unfortunately that will break some
> other parts of MusE which depend on comparing only position. (Clone parts
> must be allowed to have different lengths etc.)
> So I propose adding a term "... && lenTick() == other->lenTick()..."
> to the block above in ::isSimilarTo(). Or, any better ideas...?
MusE relying on PosLen::operator== *not* comparing length is a semantic
error. Operator== should do what it looks like, i.e. telling me if two
things are equal. and { pos=0 len=123 } and { pos=0 len=0 } are NOT equal.
(*)
>
> Also, comparing lengthy sysex data could prove to be a costly operation.
> See what I mean about being slow? Speaking of slow...
I thing UUIDs would fix that. (But still, hey, we've got 2014, the
slowest machine I own can compare 1 Megabyte in 20milliseconds!)
>
> Also, there is currently a bug where selecting notes in a clone piano roll
> does not select them in another clone's piano roll:
>
> void CItem::setSelected(bool f) {
> _event.empty() ? _part->setSelected(f) : _event.setSelected(f);
> }
That's not thread-safe anyway. (*) You're supposed to use
applyOperation() and the UndoOp::SelectEvent type instead, because *all*
changes *must* go through the audio thread. And selections should be
undoable anyway.
And then it's trivial to also select clones (I decided against that
because... well, matter of taste. Change it if you want).
>
> See what you gotta do there?
> You might want to give class Part some methods which allow other things
> to add/delete/modify events in event lists which are contained by a Part.
> In these methods, traverse the clone chain and do the task on all the
> relevant events. So, above:
> ... _event.empty() ? _part->setSelected(f) : _part->setSelected(_event);
> You could break out each Event method into a method in Part, for example
> a Part::setSelected(const Event&). But... that's several methods.
> Meaning likely you'll want a more general 'changeEvent(const Event&)'
> Either way, we must pass an event.
Well, the concept of passing Events (*) is broken anyway.
First, Event is nothing but a smart pointer which is replicating all API
of EventBase. Rather just give us access to the thing pointed to, and
operate on that's methods. And then use a sane programming language
which allows for co-variance, so a WaveEvent-smartpointer is a
descendant of a Event-smartptr.
And what you mention is already done in executeOpGroup, nicely
deduplicated. You aren't supposed to touch Parts' eventlists anyway
directly, send the audio thread a message.
>
> But... Since class Part now *owns* the event list, I'd normally say it'd be
> crazy NOT to give class EventList a parent Part pointer, optional
> of course, can be NULL like say QWidget's parent.
Children should never know about their parents. If they need to, then
your design is flawed. (*, we got this in many places, so our design IS
flawed)
> This way all such add/delete/modify event operations could be
> caught at the lowest level: The EventList class itself would take care of
> the clones. (After all, that's /almost/ like it was before, when it was
> 'automatically' shared.)
But would it also take care of updating controller values etc? No it
wouldn't, and if it did, that would be REALLY awful.
> But it's just an idea, not sure if we want a Part* in EventList. Maybe.
> It is guaranteed to catch all add/delete/modify to ensure that two
> event lists are indeed 'identical' provided the test for 'equality' works.
> Override all the STL insert()/erase() etc. to catch everything.
> Allow to pass a 'bool doClones' for doing a single part, not its clones.
> Perhaps, for speed, add more methods allowing entire lists of events
> to be passed so that that clone chain traversal goes in 'bursts' rather
> than one-at-a-time. (Benefit from STL 'insert at' methods?)
>
> This way EventLists could be /much/ more tightly linked together.
>
> All of this stuff just to support unique wave file handles, eh?
this, and for being able to remove doClones and portCtrl flags in
virtually every operation.
> Hmm...
>
> 2) Undo system, operations, and the fix of .... "The HACK"
> ---------------------------------------------------------------------------------------
> Nice work Flo. Especially separating the trivial Part Name and stuff.
> That's what I did with Track name and trivial parameters.
> No need to push complete objects onto the undo stack, just simple
> 'delta' operations.
Yes! For each and every single property this should be done (*)
>
> However, I think you goofed in one aspect, causing an observed bug:
>
> Song::executeOperationGroup2() {
> ...
> case UndoOp::ModifyPartTick: // TODO FIXME (?) do port ctrls/clones?
> editable_part->setTick(i->old_partlen_or_tick);
> updateFlags |= SC_PART_MODIFIED;
> break;
> ... }
>
> Song::revertOperationGroup2() {
> ...
> case UndoOp::ModifyPartTick: // TODO FIXME (?) do port ctrls/clones?
> editable_part->setTick(i->new_partlen_or_tick);
> updateFlags |= SC_PART_MODIFIED;
> break;
> ...}
>
> You can't do that!
> That causes a current bug where (wave) parts do not play after being moved.
> You forgot that you must erase and re-add the part in its parent STL map
> because the map sorting index is the part position.
> So a revert is in order there, back to the old modifyPart style code.
No. We should just remove and re-add the pointer (actually just re-chain
it in the tree, if map would support that). Part duplication has always
been evil (*), but now with the clone part changes it must be avoided at
all cost.
>
> And, ahem... "TODO FIXME (?) do port ctrls/clones?"
> Yes.
> Current bug: After you move a midi part around which has some
> controller graphs, the /actual/ controller values sent from the driver
> are /incorrect/ because the cached MidiCtrlValListList, found in each
> MidiPort, has not been updated with calls to removePortCtrlEvents(),
> addPortCtrlEvents() etc.
As i mentioned above, we should remove this port ctrl/clones stuff.
Since each clone is treated separately now anyway, we can conveniently
handle this kind of stuff for each clone.
>
>
> 3) Use of embedded objects instead of allocating pointers to them
> ---------------------------------------------------------------------------------------
> OK this one's kind of academic but in class Part:
> protected:
> EventList _events;
>
> I would like to change that to a pointer and allocate and destroy it
> in Part ctor and dtor. (No functional changes.)
>
> One reason: Using pointers allows to forward declare without
> including a header file. Like we do for many others:
> namespace MusECore {
> class EventList;
> ... }
> I'm actually gradually trying to /eliminate/ unnecessary header includes,
> by converting some, not all, embedded objects to pointers.
Meh. Better fix: use a sane programming language. But yeah, that
actually sounds fine.
But be sure to retain the const-ness of events()!
>
> Another reason: This is causing big headaches attempting to merge
> my code old which is expecting /pointers/ to EventList, for both
> the object itself and the two getter methods events() and cevents().
>
> -----------------------------
> Well that's all I can remember for now.
>
> PS: About the audio_streams branch: Two questions:
>
> Will this branch allow to precisely fix an object's position at an exact
> FRAME
> or an exact TIME value, and not be affected by tempo? (No = bad).
Yes. "Old style wave part behaviour" can be re-enabled per part,
actually bringing the "lock part" column in the arranger to live.
> Will the time stretchers be bypassed if no shift is required, or will they be
> on all the time even if no tempo difference exists? (Bad).
Currently No. That's impossible, because there could be a tempo change
in the future (the user could add one while playing), and then we have
trouble to get a stretcher ready, setup, prefilled with data and the
most important: in the equal phase as current playback, so the
"transition" does not cause pops.
One could probably, but not sure if it's worth the effort. RubberBand
should be able to silently pass-through at a stretch rate of 1.0 anyway
without quality loss.
>
Am 15.01.2014 03:41, schrieb Tim E. Real:> Forgot another thing: In
undo.cpp :
>
> void prepareOperationGroup(Undo& group) {
> ...
> if (op->type==UndoOp::AddEvent) // we need to clone the event
> newop = UndoOp(UndoOp::AddEvent, op->nEvent.clone(), it, op->doCtrls,
> op->doClones);
> else if (op->type==UndoOp::DeleteEvent)
> newop = UndoOp(UndoOp::DeleteEvent, it->events().findSimilar(
> op->nEvent)->second, it, op->doCtrls, op->doClones);
> else if (op->type==UndoOp::ModifyEvent)
> newop = UndoOp(UndoOp::ModifyEvent, op->nEvent.clone(),
> it->events().findSimilar(op->oEvent)->second, it, op->doCtrls,
> op->doClones);
> ...
> }
>
> I see findSimilar()->second up there.
>
> But findSimilar may return end().
>
> So taking end()->second will *crash*, right?
>
it would. But because they're clones, there HAS to exist such a thing,
so findSimilar will never return end() (in case it's not broken).
(And i feel it's a good thing when stuff crashes in case of an error,
instead of silenty ignoring it)
I marked some points with (*). See how BROKEN MusE is actually? I'd
guess that 10% of MusE are dead, unfinished or obsolete code, and at
least 40% of MusE consist of dirty hacks to work around the one or other
design flaw.
I don't want to say that anyone designing MusE has created flawed
design, no, I'm sure most design decisions were the right thing[tm] at
their time. But time changes, and what MusE should do changes, and
once-good design decisions simply don't work any more, leading to more
and more dirty hacks, which consume a multiple of the time to develop
than one would have spent with a fresh, clean design.
And leading to toilet paper programming. You know:
Four soft wrapper layers cover the smell below.
GUI uses applyOperation uses a thread messaging system uses
Part::doThings uses Event::doStuff uses EventBase::doStuff.
what the actual hell?!
Long story short: I more and more think that we should write down what
we actually expect from MusE, and then redesign it from scratch. Really.
Rebuilding MusE can't be harder than fixing current MusE.
I'm serious.
But until then, I am convinced that these changes are needed and sane.
They might still have bugs, they might be yet incomplete, but they're
the right way [tm]. Unquestionable, the branch has to mature a lot until
then.
But with MusE2, it's not a matter of "how to get it clean", it's a
matter of "how to get it without MusE exploding in my face" :(
Cheers,
flo
> Thanks.
> Tim.
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________ Lmuse-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/lmuse-developer
