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

Move rotate attribute to att.coordinated #681

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

JRegimbal
Copy link
Contributor

This makes @rotate apply to all elements with the att.coordinated class and not just the zone element. This expands it to the surface and symbolDef elements.

See #674 for more information on @rotate.

This makes rotate apply to all elements with the att.coordinated class
and not just the zone element. This expands it to the surface and
symbolDef elements.
@lpugin lpugin requested review from bwbohl and kepper June 26, 2020 09:37
Copy link
Member

@kepper kepper left a comment

Choose a reason for hiding this comment

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

I think in general it's better to move this up to att.coordinated. I'm not perfectly sure if I'd prefer to see it in a separate sub-class or not. Normally, I would, but considering where this occurs, I think it's fine to put placement and rotation in one class. What I'm more concerned with is the description. @JRegimbal's clarification in a18fee0 helped, but didn't fully fix it in my perception. Ideally, we should have an image somewhere in the documentation, both in the Guidelines and here as a remark (compare with https://music-encoding.org/guidelines/v4/data-types/data.accidental.written.html). It should be absolutely clear what the center of the rotation is, and if the two points encoded by ulx/uly and lrx/lry are already rotated (so that width and height of that rect can't be derived directly anymore, but one could add att.dimensions to compensate). I'd really like to see more explicit statements about all this before putting it in, even though it's certainly going in the right direction :-) 

@JRegimbal
Copy link
Contributor Author

My impression of how @rotate would apply to the other two elements is that, like in zone, the bounding box defined by ulx/uly/lrx/lry would not be impacted by rotation but would inform the user or software that the content should be interpreted as rotated.

So for something like <surface rotation="45">...</surface> that would mean that the entire surface described (including any zones in it) should by default be interpreted as being captured at a 45° angle. Similarly <symbolDef rotation="15">...</symbolDef> would mean that the symbol being defined should be interpreted as rotated 15° from whatever it's normal orientation would be. But in both cases the x-y plane would be unaffected.

Does this seem like a sensible interpretation to you @lpugin and @kepper? If so I can add another commit making this explicit.

Copy link
Member

@bwbohl bwbohl left a comment

Choose a reason for hiding this comment

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

Unfortunately I missed the addition of @rotate in the first place… At the moment I'm in eager need of some real world examples for @rotate. As of the name (picking up the question is it the zone or the content that are being rotated) why not pick @rotated which makes it clear in the current facsimile context. (then why limit it to theses facsimile elements? What about born digital documents?)
Concerning symbolDef I'd see more sense in adding@rotate(d) to symbolas otherwise one would need multiple symbolDef just to encode a normal and rotated version of a symbol.

@@ -689,6 +689,19 @@
<rng:data type="nonNegativeInteger"/>
</datatype>
</attDef>
<attDef ident="rotate">
<desc>
Indicates the amount by which the contents of this zone have been rotated clockwise, with respect
Copy link
Member

Choose a reason for hiding this comment

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

The description is still referring to mei:zone if @rotate is to be moved to att.rotatedthis should be generalised

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks nice I missed that!

@@ -7079,7 +7092,7 @@
<constraint>
<!-- See http://vocab.org/frbr/core for more-precise entity-to-entity constraints -->
<sch:rule
context="mei:relationList/mei:relation[parent::mei:work or parent::mei:expression or
context="mei:relationList/mei:relation[parent::mei:work or parent::mei:expression or
Copy link
Member

Choose a reason for hiding this comment

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

It's nice that this PR fixes those trailing whitespaces but for the sake of reviewing it would be better not to include such changes ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry about that my editor is very aggressive with trailing whitespace.

@lpugin lpugin requested review from bwbohl and kepper August 7, 2020 12:34
@lpugin
Copy link
Member

lpugin commented Aug 31, 2020

Anything wrong with this PR? (This is currently blocking the update of the Schema within Verovio)

@bwbohl bwbohl added this to the MEI 4.1.0 milestone Aug 31, 2020
@bwbohl bwbohl added Component: Core Schema changes to source/modules/* (assigned automatically) Status: Ready To Merge indicates that a pull request is ready for merging Type: Feature Request indicates that new features, that do not break backward compatibility, have been proposed labels Aug 31, 2020
@bwbohl bwbohl self-assigned this Aug 31, 2020
@bwbohl
Copy link
Member

bwbohl commented Aug 31, 2020

need a small fix in the documentation I'd say, sorry for the delay: music-encoding/guidelines#178

@lpugin
Copy link
Member

lpugin commented Sep 22, 2020

How does it look now?

Copy link
Member

@kepper kepper left a comment

Choose a reason for hiding this comment

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

Apologies for the long wait – this is definitely ready for merge, just as music-encoding/guidelines#178 (to be done tomorrow at the ODD Friday…)

@kepper kepper merged commit c020105 into music-encoding:develop Oct 16, 2020
@musicEnfanthen musicEnfanthen removed the Status: Ready To Merge indicates that a pull request is ready for merging label Jan 28, 2022
@rettinghaus
Copy link
Member

Could we revert this particular change? (Yes, I know I'm late to the party.) I don't see the sense in having @rotate on surface. The encoding always follow the main text direction. You would not encode music/text upside down, just because you're too lazy to rotate the folio in front of you.
Same for symbolDef. Why would you define a symbol in a rotated state? If one symbol seems to be rotated on the page you would still use zone, and that seems to be sufficient.

@ahankinson
Copy link
Member

The intention was to be able to capture the offset of the surface from the main text direction. If an image was rotated by 2° during digitization there was no way to capture this such that the notation and the surface could be aligned. You could capture width and height.

It's explained here: #620

@ahankinson
Copy link
Member

In fact, looking back at that you agreed with this, and actually suggested @rotate?

@rettinghaus
Copy link
Member

Then a zone of the size of the surface is rotated. (Or graphic, not the surface itself).
I agreed with #674.

Connected to this is actually #971.

@ahankinson
Copy link
Member

graphic points to an image file, which cannot be rotated by definition, since all image files assume 90° corners.

zone is an area of the surface. We do not allow nested zones, and I agree with the reasons why. It complicates the parsing for very little benefit.

surface is the abstract space onto which a graphic is projected, and allows the co-ordinates of the zones to be addressed. It allows for a global rotation such that all zones on that surface may be adjusted per this co-ordinate space, if the user desires.

If it helps you can think of a surface as a specialized zone -- naming it differently means we can enforce a single layer of nesting, since otherwise we would be forced to always allow zone within zone to arbitrary depths.

@bwbohl bwbohl modified the milestones: MEI 4.1.0, MEI 5.0.0 May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Schema changes to source/modules/* (assigned automatically) Type: Feature Request indicates that new features, that do not break backward compatibility, have been proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants