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.
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.
Exhibit A: The test for 'equality' of two events during clone part searches:
MidiEventBase::isSimilarTo() {
const MidiEventBase* other = dynamic_cast<const MidiEventBase*>(&other_);
if (other==NULL)
return false;
if ((a==other->a && b==other->b && c==other->c && edata.dataLen==
other->edata.dataLen && this->PosLen::operator==(*other)) == false)
return false;
if (edata.dataLen > 0)
return (memcmp(edata.data, other->edata.data, edata.dataLen) == 0);
else
return true;
}
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.
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.
(Simply by adding declaration 'MidiInstrument::virtual bool isSynti()' in my
latest commit, class SynthI overrides it, and I killed a few dynamic_cast.)
So in this case, the first line above should be safely changed to something
like this:
switch((other_.type()) {
case Note:
case Controller:
case Sysex:
case Meta:
break;
case Wave:
return false;
default:
printf("Error: Somebody please update this switch!\n");
return false;
}
const MidiEventBase* other = static_cast<const MidiEventBase*>(&other_);
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...?
Also, comparing lengthy sysex data could prove to be a costly operation.
See what I mean about being slow? Speaking of slow...
WaveEventBase::isSimilarTo() {
const WaveEventBase* other = dynamic_cast<const WaveEventBase*>(&other_);
if (other==NULL)
return false;
return f.dirPath()==other->f.dirPath() && _spos==other->_spos &&
this->PosLen::operator==(*other);
}
Ditto for what I said above about dynamic_cast and operator==.
And here, file paths might be long, so please put the "f.dirPath()==..."
comparison at the end of that line.
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);
}
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.
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.
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 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?
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.
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.
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.
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.
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).
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).
Thanks.
Tim.
------------------------------------------------------------------------------
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
