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

Add Quake II MD2 model format #619

Merged
merged 22 commits into from
Sep 6, 2023
Merged

Conversation

jdjakub
Copy link
Contributor

@jdjakub jdjakub commented Jul 19, 2022

I've been working with this format recently and saw it was missing, so I plugged the gap! I've followed the syle guide as far as I can tell, though being new to Kaitai I don't know what I should do with the overfull conditional expression lines.

You can find examples as tris.md2 under http://tastyspleen.net/~quake2/baseq2/players/. A nice, simple example is crate/tris.md2 which is just standard box geometry repeated across all frames.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

Overall, this PR is of pretty good quality.

game/md2.ksy Outdated Show resolved Hide resolved
game/md2.ksy Outdated Show resolved Hide resolved
game/md2.ksy Outdated Show resolved Hide resolved
game/md2.ksy Outdated Show resolved Hide resolved
Added documentation on standard animation frames. Renamed to quake2_md2. Used list doc-ref and `strz`.
for different animations. Each frame has a standard `name` for humans, but the
client just uses their index and the name can be arbitrary. The frames are:

INDEX NAME SUFFIX NOTES
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this section, formatted the table like this from the POV of someone reading a text file. However, now I think about it, will the doc section be rendered as Markdown in a web page? In which case, I should make this a Markdown table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this a Markdown table so it should display correctly. I think this PR is ready to merge.

Copy link
Member

Choose a reason for hiding this comment

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

@jdjakub:

will the doc section be rendered as Markdown in a web page?

Yes. The top-level doc key is rendered at https://formats.kaitai.io/ as the format description.

I've made this a Markdown table so it should display correctly.

Unfortunately, the Markdown dialect we use is CommonMark, which doesn't provide markup for tables. So you could use either use HTML syntax (<table> etc. - it would probably lose the readability a bit), or you may insert a plaintext code block ``` with an ASCII-formatted table.

ASCII table is fine, if you ask me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've now expanded the table as a preformatted block. Do you have any thoughts on my Sep 21 comment below beginning "I have added this machine-readable version of part of the animation table..."?

gl_primitive:
0: triangle_strip
1: triangle_fan
anim_start_index:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added this machine-readable version of part of the animation table. I would also like to include machine-readable data for how many frames each anim has, but I can't see how to do this in Kaitai. For example, the three pain anims all have 4 frames, which would clash if I tried to use another enum for this purpose. Is there any way to include constant key-value definitions where several keys might share the same value?

Copy link
Member

Choose a reason for hiding this comment

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

@jdjakub:

For example, the three pain anims all have 4 frames, which would clash if I tried to use another enum for this purpose.

Yes, an enum in Kaitai Struct is a strictly bidirectional mapping (one-to-one correspondence).

Is there any way to include constant key-value definitions where several keys might share the same value?

You can use value instances to define constants:

instances:
  my_const:
    value: 4

Technically you can also declare arrays there (though I'm not sure whether it makes sense here):

instances:
  my_array:
    value: |
      [2, 3, 5, 7, 11, 13]

Another option (but it depends on what you mean by machine-readable) may be to add a custom YAML key somewhere. KS compiler ignores any properties starting with a dash - (see https://doc.kaitai.io/user_guide.html#orig-id), so this can be used to add arbitrary metadata. But these keys won't be provided in generated parsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I have replaced the enum with three value instances and updated the doc to refer to them. If it's OK then please go ahead and merge, I want to be able to look it up on the website :)

game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
@jdjakub
Copy link
Contributor Author

jdjakub commented Feb 20, 2023

I've incorporated your suggested changes.

@jdjakub jdjakub requested a review from generalmimon February 21, 2023 13:34
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Show resolved Hide resolved
game/quake2_md2.ksy Show resolved Hide resolved
game/quake2_md2.ksy Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
@generalmimon generalmimon changed the title Add QuakeII MD2 model format Add Quake II MD2 model format Mar 16, 2023
@jdjakub jdjakub requested a review from generalmimon April 11, 2023 17:59
@jdjakub
Copy link
Contributor Author

jdjakub commented Apr 11, 2023

Thanks for your detailed dive into all the issues. I've incorporated your changes. Back to you :)

game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
@jdjakub jdjakub requested a review from generalmimon April 14, 2023 13:09
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
game/quake2_md2.ksy Outdated Show resolved Hide resolved
@jdjakub jdjakub requested a review from generalmimon April 14, 2023 13:20
game/quake2_md2.ksy Outdated Show resolved Hide resolved
@jdjakub jdjakub requested a review from generalmimon April 21, 2023 15:23
@jdjakub
Copy link
Contributor Author

jdjakub commented Sep 4, 2023

Hello! I think this is mergeable now.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

@jdjakub Looks good to me, thanks for your hard work on this spec and the reminder!

@generalmimon generalmimon merged commit ac6328c into kaitai-io:master Sep 6, 2023
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