markusschloesser / MackieC4_P3

A Mackie C4 Midi Remote Script for Ableton 11
20 stars 2 forks source link

last device deletion causes error #1

Closed markusschloesser closed 3 years ago

markusschloesser commented 3 years ago

When the last device of a track is deleted, the script goes crazy with an index error and I THINK the main problem is this:

RemoteScriptError: s.build_midi_map(midi_map_handle)

RemoteScriptError:   File "C:\ProgramData\Ableton\Live 11 Beta\Resources\MIDI Remote Scripts\MackieC4\Encoders.py", line 74, in build_midi_map

RemoteScriptError: Live.MidiMap.map_midi_cc_with_feedback_map(midi_map_handle, param, 0, encoder, Live.MidiMap.MapMode.relative_signed_bit, feeback_rule, needs_takeover)

RemoteScriptError: Boost.Python

RemoteScriptError: ArgumentError

RemoteScriptError: Python argument types in
        MidiMap.map_midi_cc_with_feedback_map(int, DeviceParameter, int, int, MapMode, CCFeedbackRule, bool)
        did not match C++ signature:
        map_midi_cc_with_feedback_map(unsigned int midi_map_handle, class TPyHandle<class ATimeableValue> parameter, int midi_channel, int controller_number, enum NRemoteMapperTypes::TControllerMapMode map_mode, class NPythonMidiMap::TCCFeedbackRule feedback_rule, bool avoid_takeover, float sensitivity=1.0) 

Unfortunately I do not know how to fix this currently (even though I probably spent 6 hrs looking into this already) and I think it's due to LOM changes (Live 8 => Live11)

Update: adding the "sensitivity" to the midi map signature thing improves things, see commit notes. Careful! If you delete the last device and then select the device again on the C4, the log file will fill up VERY quickly.

BilldarBagdar commented 3 years ago

This issue might be the result of a different error. When a track is deleted the "track_deleted" method will be called. The "track_removed" method takes the index of the removed track as input and updates the "list db" lists by removing the index and resorting the list. We should look carefully at that method to make sure it handles "all cases" correctly. It seems to handle "interior" Track deletions ok, but not the "last track" on the right. Why would that be? "Off by one" is a very common programming problem that could be in effect here. Especially because the track_deleted method doesn't use the track_index input parameter value that is passed in. The body of the method is relying on the accuracy of existing data

        for t in range(self.t_current + 1, self.t_count, 1):
            for d in range(self.t_d_count[t]):
                self.t_d_p_count[(t - 1)][d] = self.t_d_p_count[t][d]
                self.t_d_p_bank_count[(t - 1)][d] = self.t_d_p_bank_count[t][d]
                self.t_d_p_bank_current[(t - 1)][d] = self.t_d_p_bank_current[t][d]

            self.t_d_count[t - 1] = self.t_d_count[t]
            self.t_d_current[t - 1] = self.t_d_current[t]
            self.t_d_bank_count[t - 1] = self.t_d_bank_count[t]
            self.t_d_bank_current[t - 1] = self.t_d_bank_current[t]

This nested loop is best explained with small values I think. If you have 4 tracks correctly tracked in this "list db", and one is deleted, then this nested for loop just does a "left shift" of the data. Say the 2nd track is deleted. Before Track 2 can be deleted it will be selected, so the "current track" is 2 at the time "track deleted" is called. What the loops above do is assume self.t_current already points to the deleted track (which it does) and then starting from "t_current + 1" (the next track"), copies the data down one index. So if the lists contained data about tracks 1, 2, 3, 4 before track 2 was deleted and track_deleted was called, the lists contain data about tracks 1, (2=3-1), (3=4-1) after track_deleted returned.

Now, imagine that the only track is deleted (or the last track), which track is at index t_current +1?

I suspect you don't see the error reported until "rebuild_midi_map()" because the code gets into the wrong branch, the else branch here

        if len(self.selected_track.devices) == 0:
            self.__chosen_plugin = None
            self.__reorder_parameters()
        else:
            selected_device = self.selected_track.devices[self.t_d_current[self.t_current]]
            self.__chosen_plugin = selected_device
            self.__reorder_parameters()
        self.__reassign_encoder_parameters()
        self.request_rebuild_midi_map()
        return

I mean, if "_chosen_plugin is None" as in self.__chosen_plugin = None I don't think you would see the boost midi mapping error. But I could still be wrong.

BilldarBagdar commented 3 years ago

I spent the whole time talking about "track deleted" and you were talking about "device deleted", but IIRC, the same "off by one" is in play because "device_deleted" does the same kind of "data shifting" in the lists

markusschloesser commented 3 years ago

btw saw in the LOM docs today, that there is ALWAYS one track AND the master track in the LOM, but yes I was talking about devices 😁 Will need to read properly (and understand) tomorrow

BilldarBagdar commented 3 years ago

Good to know those are dependable facts. Some DAWs would only "guarantee" a master track exists.

Do you have any questions about the device_added_deleted_or_changed(self) method in EncoderController? I think the "device added" and "(selected) device changed" sections of that method work reliably as far as we know, and this problem seems to be only when the "last device" is deleted. Which leads me to suspect "off by one" somewhere.

I think I see the problem. This looks like the "last device guard". (line 335)

if deleted_device_index == device_count_track - 1:
    self.t_d_current[self.t_current] -= 1
else:
    self.t_d_current[self.t_current] = deleted_device_index
    sum_nbr = math.ceil((self.t_d_current[self.t_current] + 1) / SETUP_DB_DEVICE_BANK_SIZE)
    self.t_d_bank_current[self.t_current] = sum_nbr

I don't know how familiar you are with the kind of indexing I've been talking about. But generally in programmatic terms "arrays" and collections like them are indexed from zero based element addresses, which makes the "length" of any array one more than the index of it's last/largest element. If an array is length 3, it has 3 elements and those elements are indexed by addresses 0, 1, and 2. I think device_count_track is an "array length" value and device_count_track - 1 means the last valid index. So, the If statement if deleted_device_index == device_count_track - 1: is guarding the special handling condition that is supposed to account for the case when "the last device" is deleted. What is the special handling? First look at the normal handling. When it's not the last device, the Else code assigns the deleted device index to the current (selected?) device self.t_d_current[self.t_current] = deleted_device_index and when it is the last device, that same assignment is just -= 1. (shift left one index)

Next, a few lines down we see this self.__chosen_plugin = self.selected_track.devices[deleted_device_index] ...and this is where I suspect the problem could be found/fixed. When it's not the last device getting deleted, we know there's a Plugin at .selected_track.devices[deleted_device_index] because we know self.t_d_current[self.t_current] = deleted_device_index (and because we know this code works correctly when it's not the last device being deleted). But when it is the last device being deleted, all we know about that is self.t_d_current[self.t_current] -= 1 and .devices[deleted_device_index] looks very "off by one" suspicious in the "last device" case.

It helps me to think of arrays and array indexes like a whole number line starting at 0 and going off to infinity or any length in between. The general technique in this script is to treat the 128 sized lists as a more or less permanent frame of "index addresses", so if 4 devices are loaded on a track there are 124 "empty index addresses" in the "track's device list in the list db". On the number line, when a device is added at position 2, then all the devices from position 2 on up in the list "shift right" by one to make room and then the new data at position 2 is inserted. And when a device is deleted at position 2, position 2 remains, but the data is overwritten by the data "shifting left" from position 3. (In CS terms the script performs "in place" updates to the lists. Rather than using memory and processor cycles to rebuild a modified data structure in memory, this code uses only processor cycles to move data around an existing data structure in memory.) So what needs to happen when something is deleted is whatever data "shifts in from the right" to take the place of the data that was deleted (that formerly occupied that index 2) needs to be valid. If instead garbage "shifts in from the right" then garbage will fall out. I suspect when the last device is deleted, there is not valid data "shifting in from the right" to cover the spot

markusschloesser commented 3 years ago

I actually understood that explanation at the end 🤯😂 reading all those python tutorials is starting to pay off.

Couple of questions (feel free to ignore if too much):

  1. Is
    
    new_device_index = 0

deleted_device_index = 0

changed_device_index = 0


the POSITION of the (new, deleted, changed) device in the "complete devices index" 

2. Why even make/maintain a  `deleted_device_index etc ?` Why not just use a the "completes devices index" and do the in-place stuff directly in there?
3.  why not use the "listener" stuff for all that?
4.  Doesn't `            if deleted_device_index == device_count_track - 1:
                self.t_d_current[self.t_current] -= 1` do exactly the wrong thing here? Isn't that basically a "if 0 = 0-1 then do 0-1 which equals -1"? And `self.__chosen_plugin = self.selected_track.devices[deleted_device_index]` cannot do shit because there's nothing there anymore? 
At one point last week, I actually had a state of the script, that when I deleted the last device it still threw ONE error (the boost argument thing) but didn't go crazy in "ondisplaytimer" but just quietly switched to "chann strip" mode. Only trying to access the deleted device again by using the encoder press, was a bad idea. I, unfortunately don't know what I did since then, that made it appear again (on_display crazyness).
5. The first error that get thrown is always the boost thing, which led me to suspect that that's the thing to fix.
BilldarBagdar commented 3 years ago

just found this after switch to the "recent updates" sorting.

At one point last week, I actually had a state of the script, that...

This is exactly why the liberal use of branching and committing is encouraged. With git, you can go back to that state last week and have a look around. But depending on your branching and commit history, the "key difference(s)" may or may not be easier to locate in what you see "back then" than they are for you now. Also IMHO graphical tools dedicated to showing "git repositories" (like SourceTree) or command line "git commands" are better assistants for that kind of detective work than an IDE like PyCharm. What I mean is, use the other tools to set up your checked out branch to be whatever historical commit in whatever historical branch you want it to be, and then you can open your PyCharm project normally and have a look around. If that doesn't help jar your memory enough, you can try taking "diffs" between some historic commit and any other commit, such as the HEAD in your current branch. "diffs" are generally where the difference between good "branching and committing" practices and less than ideal practices appears most strikingly. Without good filtering that good branching and committing allows, "diffs" of any significance can be way too "noisy" with spurious differences to be of easy use, sometimes of any use.

In another thread I said I made changes in "on display timer" to prevent log vomit. If you removed any of those changes, the vomit may have returned. If you didn't, then this change is the "log vomit" problem, IIRC. Where you changed the string value 'None' to the null value None for the second parameter of the return tuple.

from:  return None, 'None'
to:    return None, None

IIRC, that second parameter always needs to be a string value or "on display update" needs to handle the possibility of something else or ...log vomit. (Also, IIRC, "it's coming back to me now", the reason it is/was 'None' and not ' ' is because I was "debugging" and I wanted to know exactly when this "None" case was applying, or something like that)

1 - 4. You could definitely be onto something with these observations. When I was originally working last year, I was mostly trying to not make any "logic changes" I didn't "have to". IIRC (again), the decompiler just showed the zeroes as "magic numbers" and I (thought I could) tell by context that each magic zero actually represented those "different" indexes, so I named away the "magic numbers" and replaced the "magic number" appearances by the named versions. But, although the fact that all three are set to zero seems odd, I wasn't changing logic. Ultimately, the answer to "Why Not?" is probably going to be along the lines of Leigh was working in Live 8 and we are working in Live 10 / 11.

Doesn't if deleted_device_index == device_count_track - 1: self.t_d_current[self.t_current] -= 1 do exactly the wrong thing here?

You are correct and also possibly a little mistaken. This code if deleted_device_index == device_count_track - 1 is supposed to mean, using example numbers from three devices and the third was deleted, "since there are 3 devices, if the index of the deleted device is 2, then "the last device was deleted" so handle the update with special code, and that special code is self.t_d_current[self.t_current] -= 1 This "decrement" of the value of the t_d_current list element at the t_current index appears correct to me for a general case. (I suspect it would be good to know for certain if one can only ever delete the "current device" in Live, but it makes sense to me from the Live GUI perspective) If the last device was deleted, the index where the "current device" should be found should decrement. Again with the 3 devices, in order to delete the third/last device, you have to select it which makes it current before you can delete it. In that case alone, the "current index" needs to shift left, to decrement, because "index 2" is now "one past" the last device index (now there are only 2 devices at indexes 0 and 1), and can't be a valid "currently selected device" index. You have correctly observed self.t_d_current[self.t_current] -= 1 does not seem to account for the boundary case when the only device (which would also be the last device) is deleted. If the "index value" -1 is not somehow recognized as a special case value (and that seems implausible here), then it is always an error because list and array indexes "always" start at index zero.

Another problem I might only see because I'm only looking at this quoted code, but I only see the "current" get updated, I don't see the "count" getting updated. With the three devices above, the t_d_count (AKA the device_count_track) value should be getting a decrement from 3 to 2 or something too. (and is probably not handling the zero boundary cleanly either)

The first error that get thrown is always the boost thing, which led me to suspect that that's the thing to fix.

If the "boost thing" is also the issue with the "midi mapping" method thing, AND the "boost thing" is only happening in this case and not also any other time the encoder mapping gets updated, like selecting a different device, then the "boost thing" is a symptom and not the cause in this case. The thing to fix though will be earlier whatever "bad data" is being supplied to the "midi mapping" method that makes it spew the "boost thing" error(s). You have correctly understood that the "first error" is more important, but you need to look back from there. You look at the last good thing that was logged and find that place in the code, and then you try to figure out how to get from "here to where it goes off the rails with boost issues" and find the problem on the way. (Yet, sometimes the root of the problem is before that last good log message, so you might need to turn around a few times on the bug hunting trail)

BilldarBagdar commented 3 years ago

I just noticed this behavior. I got the idea to try it from this Issue discussion.

Navigate to any Track with no devices loaded, and then press any of the "jump to device view" encoder buttons. No log vomit, and a nice "No devices loaded on this track" message in the LCD display. (sound familiar?)

That's what we want to happen after the "only device" is deleted, wouldn't you think?

So we just need to figure out why it works the way we want when no devices were on the Track when we switched to it, but not after we delete the only device on our selected Track. What is different about those two situations?

markusschloesser commented 3 years ago

I had written a whole sermon because I thought I had fixed that, but unfortunately that wasn't true (thought the script was adding a devicelistener even when the last device was deleted). But I found another very interesting thing: When you delete the last device and you are NOT in plugin mode but in chan strip mode, then you still get ONE Index Error, but that's it. So what should happen: The C4 should just switch to chan strip mode, right? And iirc this is what I had at some point.

markusschloesser commented 3 years ago

Ok I tried

`            
if device_count_track is not None:

                self.__chosen_plugin = self.selected_track.devices[deleted_device_index]  # MS this produces an Index error, w ref to device_added_deleted, origin is cb = lambda: self.device_changestate(track, tid, type)

                self.__reorder_parameters()

                self.__reassign_encoder_parameters(for_display_only=False)

                self.request_rebuild_midi_map()

else:

                self.__assignment_mode = C4SID_CHANNEL_STRIP`

didn't help, the vomiting still takes place in on_update_display_timer and the mode is not switched

BilldarBagdar commented 3 years ago

My log showed no evidence of this issue when I was testing this scenario on my latest "dsplayParams" branch before I pushed it last night. Starting on a Track with no devices; if you add a device, it "appears" on the LCD in channel mode and you can go to plugin mode. Then if you are in plugin mode and you delete the device, the display goes to "No Device Selected...". While, if you are in channel mode and you delete the device, it just "disappears" from the LCD.

Going forward I suspect we are going to need to start giving ourselves more detailed descriptions about how to reproduce certain script behavior. For example, there could be five paths to reach some "broken behavior" that is actually only broken when approached from one specific path. Unless we can accurately describe to each other how to follow that one path to the bad behavior, the behavior could be difficult to fix. I don't mean that disparagingly, programming teams often need to define ways of talking to each other using "domain specific language" in order to work together more effectively. It doesn't have to be anything bureaucratic (except in large corporations), such a language can grow and come into use organically and that would be my first choice here. We just need to remember to include enough details for the other person to reproduce by starting from, for example, "copy the script files into the scripts folder and start Live, switch to an empty Track and...". I expect we will eventually run into some issue that exists in Live 10 but not Live 11 (or vice versa), and that situation could be very difficult to figure out definitively if we are not both very confident we are each following the same reproduction recipe when we are seeing different results from the test kitchen.

If "deleting the last/only device on a Track" for example remains an issue going forward, I suspect the issue will be encountered along fewer and fewer click-paths and we will likely need to follow those breadcrumbs closely.

markusschloesser commented 3 years ago

good thing I really know how to beta test 🤓 did you see my reproducible description? (btw we (both) should really write stuff in to the correct issues ;-)instead of misusing the discussions for that)

BilldarBagdar commented 3 years ago

I have not, but I will look for it.

yes, I agree. We need to create an organized way to work and talk about work. Even more threads like "today's repository status" that are more general places to talk about various things. Like instead of a new thread to say check out the fix in branch xxx, it's just a new post in a "standard thread" for that kind of stuff. If we create an issue, we've already discussed what the issue is and don't need to have the same discussion "in the issue thread". I'm not saying that because you just posted a new issue, not respecting "frozen" is definitely a bug. But in general, a sort of pre-issue discussion thread where any possible issue discussion first appears might be useful. Another possible such thread would be for the #MS comments / questions we want to discuss. If I want to return-comment or answer your questions or vice versa, we have a common thread for that kind of thing. We can just cut/paste the comments from PyCharm to browser and comment further or whatever.

I think threads with new posts "bubble up" to the top of the community, but finding "all the new content" can seem more time consuming with every new thread. Have you ever heard of an app called Trello? Most of the features would not be useful to us, but the concept of "cards" and having a whole tree of content for each "card" is a solid organizing tool, also it's free.

Open any card to uncover an ecosystem of checklists, due dates, attachments, conversations, and more.

something to think about anyway

markusschloesser commented 3 years ago

while I know Trello and have worked with it in the past extensively, I suggest to instead try out GitHub "Project Boards" (https://docs.github.com/en/github/managing-your-work-on-github/about-project-boards ). I have never used it, obviously, but keeping everything in one place sounds rather good to me. I also seems to have Lists, Tasks etc. What do you think?

BilldarBagdar commented 3 years ago

That looks like it could work. My only hesitation would be that it looks very tailored to "project management life-cycle" "agile coding" methodology, and might not be flexible enough to suit our less stringent needs. But we'll never know if we don't try something first. So let's give it a go.

markusschloesser commented 3 years ago

So let's give it a go.

@BilldarBagdar done, looks good

markusschloesser commented 3 years ago

https://github.com/markusschloesser/MackieC4_P3/discussions/29#discussioncomment-536844 is what happened in a discussion but should've been here: tldr: undoing device insertion of a device which uses a) timing info or b) has funny characters (don't know which is true) fucks this up. See https://github.com/markusschloesser/MackieC4_P3/discussions/29#discussioncomment-540975 it probably is characters see https://github.com/markusschloesser/MackieC4_P3/discussions/29#discussioncomment-561212

BilldarBagdar commented 3 years ago

I'm wondering if you have an update on the behavior of this issue after my most recent push to origin?

If the last/only device referred to is/was always an AutoFilter, or a device with the "same" properties, for example, then this might be resolved now. If it isn't fully resolved, I suspect it might exhibit a narrower scope now.

markusschloesser commented 3 years ago

yep, I think this is related https://github.com/markusschloesser/MackieC4_P3/issues/55#issuecomment-817395948 , also swapping /directly replacing a device vomits a lot 😂

markusschloesser commented 3 years ago

https://github.com/markusschloesser/MackieC4_P3/discussions/69#discussioncomment-656964 from stupid questions thread w re to refactoring

markusschloesser commented 3 years ago

no errors to be found 😁😁😁 even undoing and using autofilter works absolutely fine. Closing this issues 👍