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 the possibility to import course from the decomp of mk64 #316

Merged
merged 51 commits into from
Aug 3, 2024

Conversation

coco875
Copy link
Contributor

@coco875 coco875 commented Feb 29, 2024

No description provided.

Copy link
Contributor

@Dragorn421 Dragorn421 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, didn't know mk64 had an interest in fast64 :)
I know nothing about mk64 so I can provide some insights on integrating with fast64 but not on the game stuff itself

fast64_internal/mk64/__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/f3d_course_parser.py Outdated Show resolved Hide resolved
fast64_internal/mk64/f3d_course_parser.py Outdated Show resolved Hide resolved
@coco875
Copy link
Contributor Author

coco875 commented Feb 29, 2024

Thanks for the PR, didn't know mk64 had an interest in fast64 :) I know nothing about mk64 so I can provide some insights on integrating with fast64 but not on the game stuff itself

I help to the decomp by document the game here is just minimal feature so only the model but later add actor spawn and waypoint can be great but I don't know to much blender to know how I can add this and later export this

@Dragorn421
Copy link
Contributor

also other games like OoT have readmes with some documentation, like
https://github.com/Fast-64/fast64/blob/main/fast64_internal/oot/README.md

Ideally you'd create one as fast64_internal/mk64/README.md too describing the features / getting started

@MegaMech
Copy link

Just a quick thought. SM64 likely has some features that aren't relevant to mk64.

Is there a possibibility of an mk64 mode that disables stuff like geo layouts?

@coco875
Copy link
Contributor Author

coco875 commented Feb 29, 2024

Just a quick thought. SM64 likely has some features that aren't relevant to mk64.

Is there a possibibility of an mk64 mode that disables stuff like geo layouts?

when you create another categories/game it disable other thing of SM64 and OOT so it's fine

@coco875
Copy link
Contributor Author

coco875 commented Feb 29, 2024

@Dragorn421 also on blender MK64 onglets never disappear you know why ?

@Dragorn421
Copy link
Contributor

@Dragorn421 also on blender MK64 onglets never disappear you know why ?

I'll look into it, I assumed from the code that'd work

bump on #316 (comment) (version prop) btw

@coco875
Copy link
Contributor Author

coco875 commented Feb 29, 2024

I test on windows and it work and I fix two little mistake so normally all it's good, if upgrade_changed_props are fine also

@Lilaa3
Copy link
Collaborator

Lilaa3 commented Jun 20, 2024

Here is my quick review, I'd like to point out that you should probably be using snake case more for variable and function names, sticking to naming conventions would be nice :) I really would like you to consider the organization aspect i mentioned.

when I make the PR all function and name have been in camel case so I don't if it change in lastest commit

sorry I'm having a hard time understanding your sentance, english is also not my first language so no judgment from me but please re-phrase

a majority of variable and function are in camel case so I put also all new function and variable in camel case

yes but ideally we'd follow the blender standard/recommendation, so pep8 with a few naming conventions, at the end of the day this is mostly fine but moving foward its best to go with something more conventional, that's what seems to be the agreed upon idea anyways although it's not very inforced.
https://docs.blender.org/api/current/info_best_practice.html

@coco875
Copy link
Contributor Author

coco875 commented Jun 20, 2024

I know python base of naming convention but use snake case now feel weird because everything else are in camel case so I think a specialise PR to fix naming convention issue will be better then update some code with it but not all

Copy link
Collaborator

@Lilaa3 Lilaa3 left a comment

Choose a reason for hiding this comment

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

Sorry if I come across as overly nitpicky, just trying to not miss anything before aproving

fast64_internal/mk64/f3d/properties.py Outdated Show resolved Hide resolved
fast64_internal/mk64/f3d/properties.py Outdated Show resolved Hide resolved
__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/f3d/properties.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lilaa3 Lilaa3 left a comment

Choose a reason for hiding this comment

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

I also tested to make sure the pr still worked and it does :) (well if you fix the draw layer aha)

fast64_internal/mk64/__init__.py Outdated Show resolved Hide resolved
__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/__init__.py Outdated Show resolved Hide resolved
fast64_internal/mk64/f3d/properties.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@Lilaa3 Lilaa3 left a comment

Choose a reason for hiding this comment

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

I'd rename f3d_course_parser.py to mk64_model_classes.py to match oot

fast64_internal/mk64/f3d/operators.py Outdated Show resolved Hide resolved
@Lilaa3
Copy link
Collaborator

Lilaa3 commented Aug 1, 2024

Commiting my suggestion was a bad idea

Copy link
Collaborator

@Lilaa3 Lilaa3 left a comment

Choose a reason for hiding this comment

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

I'm sorry for my incompetence with this review, I wanted to approve today to not make you wait more and in hurry made a mistake, sorry for making this take so long.

@Lilaa3
Copy link
Collaborator

Lilaa3 commented Aug 1, 2024

If you want to you can get the suggestion commited, I verified the code is up to date this time and edited it, it's a minor thing so I will leave this approved

@coco875
Copy link
Contributor Author

coco875 commented Aug 1, 2024

@coco875
Copy link
Contributor Author

coco875 commented Aug 1, 2024

but now it's fixe

@Lilaa3 Lilaa3 merged commit c980f33 into Fast-64:main Aug 3, 2024
1 check passed
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