search

garcia / simfile

A modern simfile parsing & editing library for Python 3
MIT License
62 stars 7 forks source link

Should NOTES/NOTES2 be handled implicitly by SSCChart? #3

Closed garcia closed 3 years ago

garcia commented 3 years ago

Keysounded SSC charts store their note data in a NOTES2 property instead of the usual NOTES (the two properties are mutually exclusive to my knowledge). This poses an interesting design problem for simfile: should invoking .notes / ['NOTES'] on an SSCChart use the NOTES2 data when present, or should this logic be kept in NoteData?

Separation of concerns dictates that NoteData should decide where to find the note data, and SSCChart should stay free of "business logic" and act as a plain dictionary of properties. This is how the library works currently, but this functionality isn't particularly discoverable, and I can imagine a lot of users not understanding why they need to call notedata.update_chart(chart) instead of the more straightforward chart.notes = str(notedata). It would be nice if the straightforward solution just worked in all cases.

Any ideas on how to resolve this elegantly?

shakesoda commented 3 years ago

status quo sounds mostly like a footgun, i would change it so doing the obvious/expected thing works.

garcia commented 3 years ago

After much deliberation, I've decided to introduce property aliases to handle the NOTES2 dilemma. Basically, the .notes attribute on SSC charts will fall back to the NOTES2 key if it's present and NOTES isn't. Aside from enabling the straightforward chart.notes = str(notedata) solution for updating a chart's notes, this also lets me clean up a lot of key in ('NOTES', 'NOTES2') type code from various places.

This is also how I'll be implementing the FREEZES and ANIMATIONS aliases that StepMania supports. In beta 6 I hardcoded the simfile parsers to replace those keys with STOPS and BGCHANGES respectively, but property aliases are much cleaner, and they enable the dictionary keys to stay true to the original file.

The only drawback to this solution is that known properties no longer map 1:1 with their corresponding uppercase keys - in other words, chart.notes is chart['NOTES'] and similar statements are no longer always true. However, with the recent improvements to known property coverage, I'm now much less concerned about preserving the semantics of indexing; any use case that depends on the semantic meanings of simfile/chart properties should prefer to use attributes instead of indexing the underlying dict.

These changes will be introduced in beta 7 unless anyone reading this objects.