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

Stabilize: load-from-file #62

Closed
11 tasks done
jcornaz opened this issue Jun 5, 2022 · 5 comments · Fixed by #87
Closed
11 tasks done

Stabilize: load-from-file #62

jcornaz opened this issue Jun 5, 2022 · 5 comments · Fixed by #87

Comments

@jcornaz
Copy link
Owner

jcornaz commented Jun 5, 2022

The cargo feature unstable-load-from-file allows loading animation from YAML or RON files.

Eventually, this should be stabilized. The present issue is there to track what needs to be considered/done before stabilizing the feature.

It is also open to gathering comments and thoughts about the current state of the API.

Tasks

  • fix crash when one frame duration is zero
  • make it easier to write an animation file when all frames have the same duration
  • configure documentation so that the new API shows up on docs.rs
  • ron support
  • gate yaml and ron behind feature flags (in addition of the load-from-file gate)
  • document shorthand notation for ron
  • implicit_some for ron
  • remove public functions from_(yaml|ron)_(str|bytes).
    • Having a public implementation or serde::Deserialize is enough to let the user choose the format and library of their choice. Not to mention that most users would not use it directly anyway as they would use the asset loader instead.

Open questions

  • is yaml a good choice?
    • I think it is. However, I am okay to support additional formats (such as ron) and let the user choose what they prefer.
  • is the extension .animation.(yaml|yml|ron) a good choice?
    • I think it is not bad. Ultimately if it turns out to be problematic later, it would be possible to let the user choose the extension (without breaking the API)
  • is the schema sensible? Clearly understood/documented?
    • I think it is not too bad. Not to say it is perfect, but I think it doesn't block the stabilization of the feature. We may still improve it later without breaking the API.
@flipbit03
Copy link
Contributor

I'd argue that ron (Rusty Object Notation) is a (subjectively) better, prettier choice!

@jcornaz
Copy link
Owner Author

jcornaz commented Jun 20, 2022

I'd argue that ron (Rusty Object Notation) is a (subjectively) better, prettier choice!

Thanks for the input.

I am not so sure about ron. As the name suggests, I think it is meant to be intuitive for a rust programmer. But a rust programmer can create the animation from rust code. The use-case for having an asset file (as I understood it), was that it could be easier for non-programmers to edit the animation. And I think YAML is more common and intuitive for everybody, including non-programmers.

But I take the feedback nevertheless. Maybe we can add support for ron on top of yaml, and let the user choose which format they want.

@jcornaz
Copy link
Owner Author

jcornaz commented Jun 20, 2022

@flipbit03 I just added ron support, so we can try it out to see if it would be a good fit.

But to be honest, In my own opinion, I think YAML is clearly superior, at least in this context.

I am of course open to suggestions on how the ron format could be improved.

Here is a quick comparison:

Simple animation

yaml

3 lines, no parenthesis, no commas

mode: Repeat
frame_duration: 100
frames: [0, 1, 2, 3, 4]

ron

5 lines, parenthesis, commas, more indentation and Some

(
  mode: Repeat,
  frame_duration: Some(100),
  frames: [0, 1, 2, 3, 4],
)

More complex animation

yaml

8 lines, no commas, no parenthesis, no square brackets, only one level of indentation

mode:
  RepeatFrom: 1
frame_duration: 80
frames:
  - index: 0
  - index: 1
  - index: 2
    duration: 120

ron

9 lines, commas, more parenthesis, square brackets, two indentation levels, and Some

(
  mode: RepeatFrom(1),
  frame_duration: Some(80),
  frames: [
     (index: 0),
     (index: 1),
     (index: 2, duration: Some(120)),
  ],
)

@flipbit03
Copy link
Contributor

I kinda of agree with your point. Although I don't think having both can hurt and that could enable more diversity. That is, unless the cost of implementing or maintaining this extra format becomes a hurdle.

By the way, the "implicit_some" feature of Ron can make those Some()'s go away.

Have a good one, and thank you for the reply/post/comparisons!

@flipbit03
Copy link
Contributor

flipbit03 commented Jun 22, 2022

@jcornaz I forgot to mention that for some people, editing the Ron's can be easier if they are relevant-white-space sensitive.
I personally prefer RON because I can just format (and also comment) the way I want and if a key isn't indented properly, it's not an issue.

RON is definitely a good candidate, specially if you are coming from a straight JSON world where everything needs "quoting" on the key side, and also no comments.

I'm happy that we get to support both authoring styles 🙏 💯

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 a pull request may close this issue.

2 participants