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

Update YAML Version and Cargo Update #190

Merged
merged 1 commit into from
Aug 1, 2022
Merged

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Aug 1, 2022

This updates the last dep we had out of date, serde_yaml, which uses a new syntax for enum tags:

Before:

        up:
          SingleAxis:
            axis_type:
              Gamepad: LeftStickY
            positive_low: 0.1
            negative_low: -1.0

After:

        up: !SingleAxis
          axis_type: !Gamepad LeftStickY
          positive_low: 0.1
          negative_low: -1.0

It's a little different than I'm used to. I didn't know YAML had tags, I think it might be a YAML 2.0 feature or something, but I what I do like is that it fixes the inconsistency of using snake_case for all of the keys ( leafwing input didn't rename the fields to use snake case like we did ). Since enum tags are now using the ! syntax and that feels a little bit more consistent and valid to have them use the CamelCase for the tags.


Another note is that I had to add a custom deserialize implementation for keeping our [start_frame, end_frame] syntax for animation clip ranges. Serde deserialize implementations are a little verbose, but it didn't turn out that bad.

@zicklag zicklag requested review from odecay and 64kramsystem August 1, 2022 18:35
Comment on lines +24 to +45
up: !SingleAxis
axis_type: !Gamepad LeftStickY
positive_low: 0.1
negative_low: -1.0

left: !SingleAxis
axis_type: !Gamepad LeftStickX
positive_low: 1.0
negative_low: -0.1

down: !SingleAxis
axis_type: !Gamepad LeftStickY
positive_low: 1.0
negative_low: -0.1

right: !SingleAxis
axis_type: !Gamepad LeftStickX
positive_low: 0.1
negative_low: -1.0
flop_attack: !GamepadButton South
shoot: !GamepadButton East
throw: !GamepadButton West
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's also less lines which is nice 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, overall seems nice.

Comment on lines +55 to +95
fn deserialize_range_from_array<'de, D>(de: D) -> Result<Range<usize>, D::Error>
where
D: Deserializer<'de>,
{
de.deserialize_tuple(2, RangeVisitor)
}

struct RangeVisitor;

impl<'de> serde::de::Visitor<'de> for RangeVisitor {
type Value = Range<usize>;

fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
formatter.write_str("A sequence of 2 integers")
}

fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
where
A: SeqAccess<'de>,
{
let start: usize = if let Some(start) = seq.next_element()? {
start
} else {
return Err(serde::de::Error::invalid_length(
0,
&"a sequence with a length of 2",
));
};
let end: usize = if let Some(end) = seq.next_element()? {
end
} else {
return Err(serde::de::Error::invalid_length(
1,
&"a sequence with a length of 2",
));
};

Ok(start..end)
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This reminds me of something I was wondering about before, which is not entirely relevant here, but I wonder if we can implement clips/animations which play a non-contiguous array of frame indexes. I had a usecase for it before when I was adding a waiting state and it didnt really match any of the available animation frames, but I'm not sure if that would be a good design. It would let us piece together frames from a spritesheet to make "ad-hoc" animations of the correct length, or skip frames in the middle of an animation if we needed to. The more I write it out the more janky it sounds though so maybe this is not a good idea 😆

Copy link
Member Author

@zicklag zicklag Aug 1, 2022

Choose a reason for hiding this comment

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

It could make sense. It would essentially be an array of ranges, which might not be bad.

Some of the exact details of the syntax become less important with an editor, too, so if it was a useful feature and we could make a good interface for it, it seems like it might be good to include. ( Not in this PR )

@zicklag
Copy link
Member Author

zicklag commented Aug 1, 2022

We good to merge this then?

@zicklag
Copy link
Member Author

zicklag commented Aug 1, 2022

bors r+

@bors bors bot merged commit 43dc808 into fishfolk:master Aug 1, 2022
@zicklag zicklag deleted the yaml-update branch August 1, 2022 19:49
@odecay
Copy link
Collaborator

odecay commented Aug 1, 2022

@zicklag this broke level loading, getting past Main Menu panics
thread 'main' panicked at 'Get value from storage: SerializationError(Error("invalid type: map, expected a Value::Tagged enum"))', src/loading.rs:358:25

reverting for now.

bors bot added a commit that referenced this pull request Aug 1, 2022
193: Revert "Update YAML Version and Cargo Update" due to broken gamestate… r=odecay a=odecay

#190 Breaks loading into the game, reverting for now.

This reverts commit a60ed4a.

Co-authored-by: otis <electricbuck@gmail.com>
@zicklag
Copy link
Member Author

zicklag commented Aug 1, 2022

Ah, I forgot about that.

The issue is that the format for the local storage file changed with the new YAML format. You just have to delete the storage YAML file and start the game again and you'll be good.

On linux the file is ~/.local/share/punchy/storage.yml.

This does bring up something we need to figure out. How do we gracefully fail when the YAML format changes or is otherwise corrupted and we can't read it?

I think the best option is to move the old storage file to storage.yml.bkup and then just re-create it with default settings. I'll open a PR to do that. On WASM, I suppose we just clear the user's data and replace it with default?

I don't really like that behavior in general, but at least while the game is unstable I feel like it makes sense.

@odecay
Copy link
Collaborator

odecay commented Aug 1, 2022

Ohh interesting, so then it wasn't really broken? I didn't think of the locally stored settings causing the issue.
What do you want to do with this now while we figure out what to do about local settings storage/format change since I reverted in #193?

@zicklag
Copy link
Member Author

zicklag commented Aug 1, 2022

I'll just open another PR that un-reverts it with an extra commit to provide a non-panicky fallback on failure to deserialize the save data.

I'm thinking the simplest solution is best for now and I'll just delete the save data if it fails to deserialize. We can re-visit when we have save data that's important, but I don't think we really have to worry about a user losing their hard-earned progress right now because there's no progress to earn. 😁 Even if there was, they could just edit the YAML of their own save data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants