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.

Sign up for GitHub

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

Jump to bottom

Change to InputEventMIDI event to include connection_index property #82157

Closed
wants to merge 5 commits into from
Closed

Change to InputEventMIDI event to include connection_index property #82157

wants to merge 5 commits into from

Conversation

ramdor
Copy link
Contributor

@ramdor ramdor commented Sep 22, 2023 •
edited by Calinou
Loading

Added connection_index to the InputEventMIDI so that each MIDI message can be associated with a specific opened MIDI device.

@ramdor
Change to InputEventMIDI event to include connection_index property
6bd6502 
Added connection_index to the InputEventMIDI so that each MIDI message can be associated with a specific opened MIDI device.
@ramdor ramdor requested review from a team as code owners September 22, 2023 23:23
@Calinou Calinou added this to the 4.x milestone Sep 22, 2023
@Calinou
Copy link
Member

Calinou commented Sep 22, 2023

Does this close godotengine/godot-proposals#7733?

@ramdor
Copy link
Contributor Author

ramdor commented Sep 22, 2023

@Calinou yes. The connection index is direcly related to the count through the midi devices that are opened.

@ramdor
clang-format update
038e1b1 
clang-format applied
@ramdor
fix for int to void*
b66be2e 
Quick fix for the compile error which I wasnt able to test locally easily. Hopefully this will now pass.
AThousandShips
AThousandShips reviewed Sep 25, 2023
View reviewed changes
drivers/alsamidi/midi_driver_alsamidi.h Outdated Show resolved Hide resolved
drivers/alsamidi/midi_driver_alsamidi.h Outdated Show resolved Hide resolved
drivers/alsamidi/midi_driver_alsamidi.cpp Outdated Show resolved Hide resolved
drivers/alsamidi/midi_driver_alsamidi.cpp Outdated Show resolved Hide resolved
core/os/midi_driver.h Outdated Show resolved Hide resolved
core/os/midi_driver.cpp Outdated Show resolved Hide resolved
core/input/input_event.h Outdated Show resolved Hide resolved
core/input/input_event.cpp Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Shouldn't this be stored in the existing InputEvent.device property instead of adding a new parameter everywhere?

@ramdor
Copy link
Contributor Author

ramdor commented Sep 25, 2023

I can make all the changes and swap it to use InputEvent.device np, removing the need for the extra parameter and fix the documentation.

@ramdor
snake_case + using device from InputEvent
3f3cfea 
Uses InputEvent.device instead, removing the need for a new property in InputEventMIDI. Updated to snake_case.
@ramdor
Copy link
Contributor Author

ramdor commented Sep 25, 2023

thanks for the review, and for the advice on using the InputEvent device instead. I have made the tweaks and submitted the changes. Cheers.

@ramdor ramdor closed this Nov 3, 2023
@AThousandShips AThousandShips removed this from the 4.x milestone Nov 3, 2023
@fbcosentino fbcosentino mentioned this pull request Dec 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AThousandShips AThousandShips Awaiting requested review from AThousandShips

Assignees
No one assigned
Labels
archived enhancement topic:input
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide MIDI device property in InputEventMIDI class
4 participants
@ramdor @Calinou @akien-mga @AThousandShips