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

[OOT] Detailed Actor Panel (Actors, Transition Actors, Entrance Actors) #242

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

Yanis002
Copy link
Contributor

Remake of PR #56 but the code is shorter by a lot, also the XML data might not be always accurate so I'll probably do some documentation (which I did a bit)

Anyway, tl;dr this is basically that tool but in Fast64, and this is a draft as most features are done, I just forgot to make a small upgrade logic and also the UI for entrances and transition actors

@Yanis002 Yanis002 marked this pull request as ready for review July 24, 2023 15:00
@Yanis002
Copy link
Contributor Author

this should be ready to review now, issues will most likely be caused by the XML documentation but it should be fine

fast64_internal/oot/oot_utility.py Outdated Show resolved Hide resolved
fast64_internal/oot/oot_utility.py Outdated Show resolved Hide resolved
"""Returns the parameter with the correct format"""
shift = getShiftFromMask(mask) if mask is not None else 0

if not int(getEvalParams(value), base=16):
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure what this means, you want to check for 0 ? == 0 would be clearer

also getEvalParams should be modified to return an int instead of a string

or do something like:

def getEvalParamsInt(...):
    ... current getEvalParams code minus stringify
def getEvalParams(...):
    i = getEvalParamsInt(...)
    return f"{i:X}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shift = shift_from_mask if there's a mask else if there's no mask then there's no shift

fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
Comment on lines 166 to 174

actorParam: StringProperty(name="Actor Parameter", default="0x0000")
rotOverride: BoolProperty(name="Override Rotation", default=False)
rotOverrideX: StringProperty(name="Rot X", default="0")
rotOverrideY: StringProperty(name="Rot Y", default="0")
rotOverrideZ: StringProperty(name="Rot Z", default="0")
rotOverrideX: StringProperty(name="Rot X", default="0x0000")
rotOverrideY: StringProperty(name="Rot Y", default="0x0000")
rotOverrideZ: StringProperty(name="Rot Z", default="0x0000")

params: StringProperty(
name="Actor Parameter",
Copy link
Contributor

Choose a reason for hiding this comment

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

could there be a comment somewhere about the difference between actorParam and params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actorParam is used for custom actors (actors with the id custom)

Copy link
Contributor

Choose a reason for hiding this comment

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

both are a StringProperty without get/set, it doesn't seem there needs to be two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

params use get/set

fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

there's upgrade code but no cur_version bump anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeaaaaahhhhhh I guess, that PR is kinda old now so I probably didn't know yet about that and end up forgetting about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I remembered why, cur_version doesn't exist anymore. we discussed about that a while ago and figured out checking if old props exist in a blend is stronger than a version check (idr the specifics though, iirc it was kure's idea)

Comment on lines 748 to 750
override = "Override" if actorProp.actorID == "Custom" else ""
overrideRots = [getattr(actorProp, f"rot{override}{rot}") for rot in ["X", "Y", "Z"]]
exportRots = [f"DEG_TO_BINANG({(rot * (180 / 0x8000)):.3f})" for rot in blendRots]
Copy link
Contributor

Choose a reason for hiding this comment

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

so there are three sets of rotations?

  • one from the blender object
  • two "override", one for non-custom actors and one for custom? why are these two different? why are the properties for "Custom" named "Override"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • blender object (which won't be used if the actor can have parameters in its rotation, which makes sense to me)
  • string prop rotation for actors without the id "custom"
  • string prop rotation for custom actors

iirc the override thing is gone and it all depend on the xml data, I fail to see how using rotations as real rotation and not parameter can be useful if the actor is using rotations

btw this depends on other parameters, some actors doesn't always actually check for the rotation for params, in this case the blender object rotation can be used (aka if you don't see any rotation param field in the fast64 ui)

@Reonu
Copy link
Contributor

Reonu commented Apr 18, 2024

@Dragorn421 I'm using this rn and it works great, it makes oot scene creation 100 times easier and it's a crime that it's not merged :(

@Yanis002
Copy link
Contributor Author

Yanis002 commented Apr 19, 2024

(just so I don't forget) todo: look into upgrade issues

edit: apparently it works?? I'm confused

@Yanis002 Yanis002 added this to the v2.2.2 milestone May 20, 2024
Copy link
Contributor

@Reonu Reonu left a comment

Choose a reason for hiding this comment

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

Will this get merged soon? I've been using it for quite some time now and it seems to have no issues (the upgrade thing was seemingly a false alarm too).

Giving it an approving review since I've been using it a lot and I can confirm it works well

@Yanis002
Copy link
Contributor Author

I added this to the next milestone a while ago so it will be merged before september for sure (hopefully)

also I don't remember if it was this PR specifically but I think I had another issue but I can't remember what is was 🥲

Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

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

Seems mostly good. However, I think Type/Enum should support custom values (ex. you want to add a new Poe type while still using the existing Poe actor), and parameters should be allowed to use string inputs (ex. MY_CUSTOM_FLAG instead of 0x01).

fast64_internal/oot/actor/operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/operators.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
fast64_internal/oot/actor/properties.py Outdated Show resolved Hide resolved
Copy link
Contributor

@kurethedead kurethedead left a comment

Choose a reason for hiding this comment

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

Some issues:

  1. For Treasure Chest / Navi Msg, choosing a custom chest content / message value causes an error.
  2. If eval params is enabled but the params fail to evaluate, I think the value should still be the unevaluated params. Currently its left blank, which results in an invalid export.
  3. For Rot XYZ + Params, if string input is used, I think masking / shifting should still be applied, so that its consistent with numerical input.

fast64_internal/oot/importer/actor.py Outdated Show resolved Hide resolved
fast64_internal/oot/importer/actor.py Outdated Show resolved Hide resolved
fast64_internal/oot/importer/actor.py Outdated Show resolved Hide resolved
fast64_internal/oot/importer/utility.py Outdated Show resolved Hide resolved
@Dragorn421 Dragorn421 modified the milestones: v2.3.0, v2.4.0 Aug 19, 2024
@Dragorn421 Dragorn421 added enhancement New feature or request oot Has to do with the Ocarina of Time 64 side labels Aug 20, 2024
@Yanis002
Copy link
Contributor Author

Yanis002 commented Dec 1, 2024

I fixed the issues you mentioned, the only thing left is this: For Rot XYZ + Params, if string input is used, I think masking / shifting should still be applied, so that its consistent with numerical input. but I don't understand what you mean here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request oot Has to do with the Ocarina of Time 64 side
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants