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 a parameter to ignore invisible objects when parsing MusicXML files #401

Merged
merged 3 commits into from
Jan 21, 2025

Conversation

leleogere
Copy link
Collaborator

@leleogere leleogere commented Nov 26, 2024

As explained in #395, it can sometimes be useful to parse a score only with information available to a human, i.e. visible information. MusicXML allow some elements to be invisible with the attribute print-object that can be set to "yes" or "no".

This PR simply add a new parameter ignore_invisible_objects to the function load_musicxml, defaulting to False for old behavior. When set to True, the loop over all elements in a measure will first check if the element has an attribute print-object set to "no", and if so, continue to the next loop without adding the object to the score (for note objects, I had to update the position with the duration anyway to avoid errors with backups when notes were not added to the part, I don't know if this can cause issues downstream?).

Note: Initially, I wanted to fully parse the print-object attribute, and adding it to all objects supporting it, but it turns out it is a bit more complex than I thought. You need to filter objects that do support it, and maybe use some sort of CanBeInvisible mixin to avoid duplicated code, then handle the attribute parsing logic in all the relative _handle_*object* when parsing the XML, probably resulting in duplicated code without some sort of refactoring somewhere. Additionally, we would need to also implement the export logic. This PR is way simpler, simply filtering invisible object at import-time.

…ject` set to "no" when parsing MusicXML files
@manoskary manoskary added the enhancement New feature or request label Nov 27, 2024
@CarlosCancino-Chacon
Copy link
Member

Hey @leleogere, thanks for the contribution! It would be great if you could add a test case for this feature (for example inpartitura/tests/test_xml.py).

@leleogere
Copy link
Collaborator Author

I'll do that this week!

@leleogere
Copy link
Collaborator Author

Added some tests!

Copy link
Member

@manoskary manoskary left a comment

Choose a reason for hiding this comment

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

This PR covers the support of invisible objects when parsing MusicXML.
Excellent contribution by @leleogere !!!

The implementation is great, but it also raises some preference questions as well:

  • Should notes be allowed to be invisible?
  • Should invisible mean skip parsing or should it be a TimedObject attribute that forces it to not be returned when part.iter_all is called and relevant functions?

As per this review, all questions are high-level and do not necessarily need to be addressed or correct the provided implementation but rather open a discussion.

tests/test_xml.py Outdated Show resolved Hide resolved
Comment on lines +555 to +556
# If the object is invisible and the user wants it, skip the object
# Will probably not skip everything, but works at least for notes and rests
Copy link
Member

Choose a reason for hiding this comment

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

This is more of a high-level question, the implementation is very fine in my opinion.

I get the concept of invisible rests, or even invisible measures/time signatures, etc. but I am not sure about the utility of invisible notes. What would be a potential use-case for this and why should partitura support skipping some notes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The most frequent case I've seen invisible notes is to simulate trills/mordents/tremolos for MIDI export (this is not needed in MuseScore as it automatically render those notes, but some other software may not? Or some people might want to end the trill in a custom way?). When parsing this kind of score, I would like to only have access to visible note (eventually with trill/mordent/tremolo information), but not to notes that have been added here for the sole purpose of MIDI export.

A lot of scores in ASAP have such invisible notes, for example in measures 21 and 23 of Chopin/Sonata_2/2nd_no_repeat/xml_score.musicxml.

Copy link
Member

@manoskary manoskary Jan 15, 2025

Choose a reason for hiding this comment

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

Thank you, I hadn't thought of such an application. This would then definitely be a great addition to partitura.
Thanks again for all your work. I will resolve this review conversation once the PR is ready for merge.

@manoskary manoskary linked an issue Jan 20, 2025 that may be closed by this pull request
Copy link
Member

@manoskary manoskary left a comment

Choose a reason for hiding this comment

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

Great addition for parsing or skipping invisible objects in musicxml. Cases for invisible notes can be spelled out trills and others. This PR includes tests too.

@sildater sildater merged commit 0122040 into CPJKU:develop Jan 21, 2025
3 checks passed
@leleogere leleogere deleted the support_print-object branch January 22, 2025 09:19
@manoskary manoskary mentioned this pull request Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
4 participants