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 Mii formats for other consoles #356

Closed
wants to merge 3 commits into from
Closed

Conversation

larsenv
Copy link

@larsenv larsenv commented Nov 17, 2020

This is related to #355, I've worked on this Kaitai with @HEYimHeroic.

The Wii U/3DS/Miitomo Kaitai in here is in a bad state because I could never figure out how to grab bits from a little-endian integer.

@larsenv
Copy link
Author

larsenv commented Nov 17, 2020

@dgelessus Please review this.

@generalmimon
Copy link
Member

@larsenv Can I ask you to provide some example files for testing?

The Wii U/3DS/Miitomo Kaitai in here is in a bad state because I could never figure out how to grab bits from a little-endian integer.

Does the section Bit-sized integers in the User Guide help?

@generalmimon
Copy link
Member

And yeah, for your own convenience, remember a rule of thumb when making pull requests from your fork repository: never commit directly on master of your fork. Create a topic branch for every PR you create. See https://contribute.jquery.org/commits-and-pull-requests/#never-commit-on-master:

When you're working on a fork, you should always think of your master branch as a "landing place" for upstream changes. You should only ever make your commits to topic branches, and your own commits should only ever end up on master after they've been merged in upstream by a maintainer.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! You've probably already seen my comments in #355, and many of them also apply to this PR, so I've kept my review comments here a bit short. If you have any questions, let me know.

About extracting bits from little-endian integers: Kaitai Struct's default behavior in this case can be confusing and doesn't work at all with some little-endian formats. The recent 0.9 release has improved support for little-endian bit-sized integers. I would recommend that you set bit-endian: le under meta in the KSY and then try to split the byte integers into bit integers again.

With bit-endian: le, you need to order the bit fields from low to high bits. If you look at the expressions that you've currently written under instances, the fields with a low number after >> (or no >> at all) should come earlier than the ones with a high number after >>. For example the fields stored in data_1 should go in the order gender, birth_month, birth_day, favorite_color, favorite.

For the full details on how Kaitai Struct's bit-sized integers behave depending on the bit-endian setting, see the documentation that @generalmimon linked above.

@@ -0,0 +1,174 @@
meta:
id: gen2_wiiu_3ds_miitomo
Copy link
Contributor

Choose a reason for hiding this comment

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

The id in meta has to match the KSY file name - see #355 (comment)

(Same for the other file.)

Comment on lines +8 to +13
- id: character_set
type: b2
doc: 0=JPN+USA+EUR, 1=CHN, 2=KOR, 3=TWN
- id: region_lock
type: b2
doc: 0=no lock, 1=JPN, 2=USA, 3=EUR
Copy link
Contributor

Choose a reason for hiding this comment

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

These values could be defined as enums.

seq:
- id: unknown_1
type: u1
doc: Always 3?
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use valid: 3 here to check that the value is really always 3 (or to make it more noticeable when you encounter data where it isn't 3). See also #355 (comment)

Comment on lines +28 to +39
- id: system_id
type: u1
repeat: expr
repeat-expr: 8
- id: avatar_id
type: u1
repeat: expr
repeat-expr: 4
- id: client_id
type: u1
repeat: expr
repeat-expr: 6
Copy link
Contributor

Choose a reason for hiding this comment

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

It's almost always better and more efficient to use just size instead of type: u1 with repeat-expr - see #355 (comment)

(Same for a few fields in the other file.)

- id: hair_color
type: b3
- id: eye
type: u4le
Copy link
Contributor

Choose a reason for hiding this comment

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

Because you have set endian: le in meta, there's no need to explicitly use u4le - you can simply use u4 instead. (Similarly for the other u4le and u2le fields below.)

mole_size:
value: mole >> 1 & 15
mole_enable:
value: mole >> 15
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor thing: there should be a newline at the end of the file.

(Same for the other file.)

- mii
- mae
- miigx
endian: be
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, this KSY doesn't use numbers larger than one byte, so you cam remove the endian setting here - see #355 (comment)

@larsenv
Copy link
Author

larsenv commented Nov 21, 2020

@generalmimon @dgelessus Thanks.

At the time when I made the Kaitai, the IDE didn't support the bit-endian attribute. It does now, so I made the changes and now it looks much better in my opinion.

And about using a branch other than master, I'm aware that making a new branch is better, but when I made the PR I quickly did it using the web client and forgot about it.

Here are 2 example files:

3DS/Wii U/Miitomo: https://transfer.notkiska.pw/NKy0p/3ds_mii.bin
Wii: https://transfer.notkiska.pw/hcilO/wii_mii.bin

These Kaitais are used to make this tool possible.

@@ -0,0 +1,178 @@
meta:
id: WiiU3DSMiitomo_miidatafile
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't going to work:

io.kaitai.struct.format.YAMLParseException: /meta/id: invalid meta ID: 'WiiU3DSMiitomo_miidatafile', expected /^[a-z][a-z0-9_]*$/

Kaitai Struct doesn't accept uppercase letters in ids, because it might need to translate the identifier depending on the target language. For example, class names are by convention written in CamelCase in many languages, so a KSY named some_long_name will be translated to a class named SomeLongName. This kind of translation becomes more complicated and unclear if the original name already has uppercase letters in it, so Kaitai Struct just forbids uppercase letters in identifiers.

Copy link
Member

Choose a reason for hiding this comment

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

Kaitai Struct doesn't accept uppercase letters in ids, because it might need to translate the identifier depending on the target language.

Yes, the reason is described in kaitai-io/kaitai_struct#148.

@larsenv
Copy link
Author

larsenv commented Nov 29, 2020

Sorry, it's been a week, this is sort of low priority for me.

Is the commit I made better? What else needs to be done?

Copy link
Contributor

@KOLANICH KOLANICH left a comment

Choose a reason for hiding this comment

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

IMHO: should be moved into a dedicated dir with all its siblings (#355), modularized and deduplicated.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

What else needs to be done?

See the code comments in my earlier review: #356 (review)

@@ -0,0 +1,178 @@
meta:
id: wiiu3dsmiitomo_miidatafile
Copy link
Contributor

Choose a reason for hiding this comment

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

The id is okay now, but the file name still has uppercase letters - it should also be all lowercase like the id.

If you're on Windows or Mac, it's possible that you've already renamed the file to lowercase and Git simply didn't detect the change (this can happen because Windows and Mac use case-insensitive filesystems by default). In that case you might have to use GitHub's web interface to rename the file to lowercase.

@@ -0,0 +1,142 @@
meta:
id: gen1_wii
Copy link
Contributor

Choose a reason for hiding this comment

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

The id and file name here also need to be changed to match.

This was referenced Jan 15, 2021
@larsenv
Copy link
Author

larsenv commented Jan 17, 2021

Going forward, we should continue with #400.

@larsenv larsenv closed this Jan 17, 2021
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.

4 participants