Conversation
|
@Darxoon is attempting to deploy a commit to the Alex Bates' projects Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bates64
left a comment
bates64
left a comment
There was a problem hiding this comment.
Cool idea, I like the user-facing results but I can't accept it as-is.
If you can repeat your changes to the canonical Bgm, and modify the frontend to work with it, I'd accept it. From what I can tell, the main changes are:
- snake_case fields (ideally do this with serde aliases, rather than renaming the fields)
enum PolyphonyOptionforparent_track_idx(which is in fact related to Polyphony - see the UI - so should maybe be included in a Polyphony struct alongside a VoiceCount enum?)
| #[wasm_bindgen] | ||
| pub fn ron_encode(bgm: &JsValue) -> JsValue { | ||
| let bgm: Bgm = from_js(bgm); | ||
| let bgm = bgm_ron::Bgm::from(bgm); |
There was a problem hiding this comment.
I don't want this line anyway, but it would be better written (imo) as let bgm: bgm_rom::Bgm = bgm.into();
| @@ -0,0 +1,655 @@ | |||
| use std::{collections::HashMap, mem::MaybeUninit}; | |||
There was a problem hiding this comment.
I can't accept a near identical copy of bgm.rs as I'd need to maintain both forever. It also makes it hard to see what you've changed unless I diff the two files.
There was a problem hiding this comment.
Even if I was OK with the file existing, there is a ton of boilerplate here e.g. trait impls that are identical for bgm_ron::Bgm and Bgm as input, would be better off making a trait for both if you wanted to do this
| pub is_disabled: bool, | ||
| pub polyphony: Polyphony, | ||
| pub is_drum_track: bool, | ||
| pub parent_track_idx: Option<u8>, |
There was a problem hiding this comment.
Option is both more correct and cooler
| #[serde(default)] | ||
| pub name: String, | ||
| pub is_disabled: bool, | ||
| pub polyphony: Polyphony, |
There was a problem hiding this comment.
Great change, make it work in bgm.rs and update js and I'll gladly accept it
| 6 => Self::Manual { voices: 3 }, | ||
| 7 => Self::Manual { voices: 4 }, | ||
| POLYPHONIC_IDX_AUTO_MAMAR => Self::Automatic, | ||
| _ => Self::Other { priority: value }, |
There was a problem hiding this comment.
Slightly unfortunate that the variants overlap eachother in u8 representation, but I guess that can't be helped
| track.polyphony = Polyphony::Manual { voices: 0 }; | ||
| } else { | ||
| track.polyphonic_idx = 1; | ||
| track.polyphony = Polyphony::Manual { voices: 1 }; |
There was a problem hiding this comment.
| track.polyphony = Polyphony::Manual { voices: 1 }; | |
| track.polyphony = Polyphony::Automatic; |
| 126 => { | ||
| if value == 0 { | ||
| track.polyphonic_idx = 0; | ||
| track.polyphony = Polyphony::Manual { voices: 0 }; |
There was a problem hiding this comment.
| track.polyphony = Polyphony::Manual { voices: 0 }; | |
| track.polyphony = Polyphony::Manual { voices: 1 }; |
|
Dry_Dry_Desert_Oasis_17 fails to match, as it uses conditional takeover. I suspect the encoded |
2 participants
This PR significantly improves the readabililty and editability of mamar's ron files