Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add MIDI controller device index to InputEventMIDI.device property. #86620

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

fbcosentino
Copy link
Contributor

It is possible to query the OS for the connected MIDI controllers, but the event messages' device field was not being used. This implements controller index being sent in InputEventMIDI messages in the device property, matching the index from OS.get_connected_midi_inputs().

Based on the work done by @ramdor in this archived PR

Closes godotengine/godot-proposals#7733

@AThousandShips
Copy link
Member

The co-author didn't apply, you need to make the part in the arrow brackets their email, check out their branch and use git log to get a valid author to use 🙂

@fbcosentino
Copy link
Contributor Author

The co-author didn't apply, you need to make the part in the arrow brackets their email, check out their branch and use git log to get a valid author to use 🙂

Done. Seems to work now.

@AThousandShips
Copy link
Member

Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@fbcosentino
Copy link
Contributor Author

Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

Fixed. (Apparently I'm as good with github as Sheldon is good with the flute. Eventually I'll get better.)

@PeterMarques
Copy link

This needs to get pushed up.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no experience with drivers, but the changes to core are looking correct.

core/input/input_event.cpp Outdated Show resolved Hide resolved
It is possible to query the OS for the connected MIDI controllers,
but the event messages' device field was not being used. This implements
controller index being sent in InputEventMIDI messages in the device
property, matching the index from OS.get_connected_midi_inputs().

Based on the work done by @ramdor.

Closes godotengine/godot-proposals#7733

Co-authored-by: Richie <[email protected]>
Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to core are looking correct.
Can't say anything about the change to drivers.
I can't test the PR, because I don't have MIDI-devices.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 19, 2024
@akien-mga akien-mga changed the title Add MIDI controller device index to InputEventMIDI.device property. Add MIDI controller device index to InputEventMIDI.device property. Feb 19, 2024
@akien-mga akien-mga merged commit 8a3a559 into godotengine:master Feb 19, 2024
16 checks passed
@ramdor
Copy link
Contributor

ramdor commented May 22, 2024

hi @fbcosentino . I know this has been merged now, based on the changes I was dabbling with a long time ago. However, there are some issues with the coremidi implementation. I built a quick mac test and it seems like the device_index (InputEvent.device) is garbage data.

It looks to be caused by line 68 of midi_drive_coremidi.cpp where the pointer to 'i' is being used. This should not be a pointer, but the actual value of 'i'. Otherwise multiple read callbacks will use the same pointer. Also 'i' will go out of scope and result in junk.

We need to provide the actual value of i to MIDIPortConnectSource so that when the callback function read is called when midi events come in, src_conn_ref_con is the actual value of i that was provided when MIDIPortConnectSource was called and can be used as device_index.

I hope this makes sense. Cheers.

@ramdor
Copy link
Contributor

ramdor commented May 22, 2024

some test code that does correct type casting :

#include <iostream>
//#include <cstdint>

void testFunction(void *src_conn_ref_con)
{
    intptr_t device_index = reinterpret_cast<intptr_t>(src_conn_ref_con);
    std::cout << device_index << std::endl;
}

int main()
{
    int sources = 16;
    for (int i = 0; i < sources; i++) {
        testFunction(reinterpret_cast<void*>(static_cast<intptr_t>(i)));
    }
}

@fbcosentino
Copy link
Contributor Author

some test code that does correct type casting :

#include <iostream>
//#include <cstdint>

void testFunction(void *src_conn_ref_con)
{
    intptr_t device_index = reinterpret_cast<intptr_t>(src_conn_ref_con);
    std::cout << device_index << std::endl;
}

int main()
{
    int sources = 16;
    for (int i = 0; i < sources; i++) {
        testFunction(reinterpret_cast<void*>(static_cast<intptr_t>(i)));
    }
}

Hi @ramdor !

I actually don't have a mac (nor access to one). Would you be comfortable to make these changes yourself?

@ramdor
Copy link
Contributor

ramdor commented May 22, 2024

@fbcosentino yes, i have just started a PR. I couldn't test it either, but it should be ok.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide MIDI device property in InputEventMIDI class
7 participants