enables Beatmaps to be written back to .osu files

[IMXZ1234]

pack function is added for HitObject and Curve as well. Written .osu has been tested and can be recognized and played by osu! client. Colours section in .osu file is omitted since slider does not parse this section. Support for parsing colours may be added in the future, so the generated .osu can include this section. Default values are chosen to be the same as those used by the Beatmap editor which is embeded in osu! client.

[IMXZ1234]

However, during the test of the generated .osu file, all HitObjects are interpreted as in a single combo. I think the problem is with the parsing phase. When parsing HitObjects, combo information which is saved together in one int with hit object type_code is dropped. This may be fixed, perhaps, by adding extra is_new_combo and combo_colour member to HitObject to record the combo information.

[tybug]

Before I take a look at this, can you fix the inspection errors? (you might want to lint locally with flake8 instead of pushing to test; from experience, fixing one inspection often causes another to pop up). Github made me manually approve the workflows to run on this pr since you're a first time contributor, which is why it didn't run when you first opened the PR.

[IMXZ1234]

Inspection errors are all E501 line too long, now fixed. Also included a few minor changes:

1. Shorten the function name _replace_invalid_with_default to _invalid_to_default.
2. Prevent Bookmarks field in Editor section from appearing in generated .osu file if it is an empty list.

[IMXZ1234]

I believe the problem in my former review boils down to this: Using default test beatmap AKINO from bless4 & CHiCO with HoneyWorks - MIIRO vs. Ai no Scenario (monstrata) [Tatoe].osu, Simply add a test in slider/tests/test_beatmap.py like this:

def test_parse(beatmap):
    print(beatmap.hit_objects()[12])

the result will be:

<Circle: Position(x=52.647999999999996, y=201.648), 4923ms>

While in the original .osu file the line recording this hit object is:

49,198,4923,5,0,2:0:0:0:

Position is incorrect.

[IMXZ1234]

Now I learn the problem can be solved by setting the stacking parameter of Beatmap.hit_objects() to False to disable the _resolve_stacking function turned on by default. Maybe changing its default to False is better?

[tybug]

I don't think stacking=False would be a good default. Even ignoring the fact that changing it would be a breaking change, most uses of slider want the position of the hitobject as it would appear in gameplay, which is what stacking provides.

[IMXZ1234]

I had written tests.test_beatmap.test_pack() as a draft script rather than a formal piece of code. To avoid writing long segments of attribute names I had turned to __dict__. Well, the code just grew grotesque. It was my fault to have valued convenience over readability. Now it has been refined, and I think it's neater now. Since pack() is a member method, I suppose directly accessing the private member _hit_objects is all right. Also, it promotes efficiency since self.hit_objects(stacking=False) does quite a number of checks inside and actually constructs a new tuple on its return. Thus I suppose it would be better to use _hit_objects instead.

[tybug]

please don't force push if you can absolutely avoid it. It messes up the history of anyone who has checked out this pr (ie, me).

[tybug]

Since pack() is a member method, I suppose directly accessing the private member _hit_objects is all right. Also, it promotes efficiency since self.hit_objects(stacking=False) does quite a number of checks inside and actually constructs a new tuple on its return. Thus I suppose it would be better to use _hit_objects instead.

generally I prefer accessing known apis (ie self.hit_objects()) where possible, even inside member methods. But on second thought this is actually probably better since you really do want the "raw" representation of the hitobjects, since you're about to write them back to a .osu file.

[tybug]

I've cleaned up the testing code, slightly opinionated-ly. The tuple(map( business in was particularly bad imo, and written more cleanly with a zip. The only behavioral changes I made were to access beatmap._hit_objects instead of beatmap.hit_objects(stacking=False) (you convinced me this was the better path forward above), and to assert something slightly stronger with the timing points:

assert (tp1.parent is not None) == (tp2.parent is not None)

instead of

        if timing_point1.parent is None:
            assert timing_point2.parent is None

which only catches the case of "tp1 parent none and tp2 parent not none", and not "tp1 parent not none and tp2 parent none".

Let me know if you hugely disagree with any of my changes. I'll do another pass on this pr soon, but the testing code looks good to me now.

[IMXZ1234]

which only catches the case of "tp1 parent none and tp2 parent not none", and not "tp1 parent not none and tp2 parent none".

Good insight. I have no further problem.

[tybug]

A few things:

• Sorry it took me so long to review this pr.

• It appears that we never parsed bookmarks properly. I've fixed this in this pr (https://github.com/llllllllll/slider/pull/101/commits/e44dd051bd7f0020880379be2ba09d0c5ec93708), and fixed the bookmark packing functionality as well (it wasn't producing correct results as it expected a list of ints and not timedeltas).

• I'm unconvinced this pack_field approach is the correct one, because it imposes strict requirements on the kind of functions it can accept as its third argument - it must take exactly three values; the field name, field value, and default. This means you can't add extra options to the individual pack functions like _pack_str_list(..., separator=" ") which is something I reached for when trying to pack bookmarks properly. But it works well enough that I'm not going to nitpick.

The largest remaining thing to do is combo colors, which as you've noted above we currently discard. I think a good path forward would be changing how we store the type_code for hitobjects by keeping the entire value of type_code from the parsed hitobject instead of just what hitobject type it is. I'll leave that for another pull, if at all.

I think this PR is good to go now. I'll wait to make sure you don't have any problems with my recent changes, and then merge.

[IMXZ1234]

I think it may be better to replace the nested function _pack_bookmarks with function _pack_timedelta_list which is generally available.

[IMXZ1234]

Custom separators can now be used, by passing partial(_pack_xx_list, sep=xx) to _pack_field. the field name, field value, and default are shared parameters among all _pack_xx functions but separator is only needed in _pack_xx_list functions. Making these three argument mandatory while keeping separator as optional sounds sensible to me. Another solution maybe using **kwargs, but I do not use that since usage of optional parameters is not common(only _pack_xx_list need optional parameters). Nonetheless, if you still find such a solution absurd, feel free to discard this change. Other than these, I think everything is well.

[tybug]

still feels like an overengineered solution (re: separators), but I'm not going to fuss too much. That may also just be my bias against this functional style of programming showing. Let's get this in as-is. Thanks for the pr!