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

Accessing Animation Data #41

Open
wants to merge 3 commits into
base: 1.20
Choose a base branch
from
Open

Conversation

BlueMoonJune
Copy link

No description provided.

Copy link
Contributor

@lenrik1589 lenrik1589 left a comment

Choose a reason for hiding this comment

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

And overall, for future reference: you can just add more commits to the branch you are pulling from (github should be showing you the prompt "Add more commits by pushing to the 1.20 branch on Ovionyx/Figura.") and there is no need to close PR and re-open as a new one

List<Animation.AnimationChannel> value = new ArrayList<Animation.AnimationChannel>();
for (Animation.AnimationChannel channel : entry.getValue()) {
value.add(channel);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any need to iterate over all channels instead of initializing the list with the collection?

Copy link
Author

Choose a reason for hiding this comment

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

idk

@@ -140,6 +141,25 @@ public void playCode(float minTime, float maxTime) {
// -- lua methods -- //


@LuaWhitelist
@LuaMethodDoc("animation.getChannels")
public LuaTable getChannels() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing this is because it is called "channel" in BlockBench?

Copy link
Author

Choose a reason for hiding this comment

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

its because thats what figura refers to them as (AnimationChannel)

name = "AnimationChannel",
value = "animation_channel"
)
public record AnimationChannel(TransformType type, Keyframe... keyframes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, does @LuaWhitelist not work on record fields?

Copy link
Author

Choose a reason for hiding this comment

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

uhhhh idk lol

Copy link
Author

Choose a reason for hiding this comment

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

i jsut forgot to add it i think

public FiguraVec3 getTargetB(float delta) {
return targetB != null ? targetB.copy() : FiguraVec3.of(parseStringData(bCode[0], delta), parseStringData(bCode[1], delta), parseStringData(bCode[2], delta));
}


Copy link
Contributor

Choose a reason for hiding this comment

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

Just an extra empty line?


"figura.docs.keyframe.get_target_a": "Gets target 'A' of this keyframe\n I have no idea what 'A' means",
"figura.docs.keyframe.get_target_b": "Gets target 'B' of this keyframe\n I have no idea what 'B' means",
"figura.docs.keyframe.ph": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

:>

This comment was marked as resolved.

@@ -83,26 +96,43 @@ private float parseStringData(String data, float delta) {
return FiguraMod.popReturnProfiler(0f);
}

@LuaWhitelist
@LuaMethodDoc("keyframe.ph")
Copy link
Contributor

Choose a reason for hiding this comment

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

@LuaMethodDoc is not required for a function to be accessible in Lua, but if you are adding docs, please do it properly or remove useless "docs".

Copy link
Author

Choose a reason for hiding this comment

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

From what i can tell thats required to have it show up in the docs command, they're all pretty self explanatory, so documentation shouldnt really be necessary imo

Copy link
Contributor

Choose a reason for hiding this comment

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

"setPos" and "setRot" are also self-explanatory but still are documented, and it's better to have consistent documentation

Copy link
Author

Choose a reason for hiding this comment

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

ugh fine ill add docs

public Interpolation getInterpolation() {
return interpolation;
}

@LuaWhitelist
@LuaMethodDoc("keyframe.ph")
public String getInterpolate() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, forgot about this, but getInterpolation (verb noun) not getInterpolate (verb verb)

Copy link
Author

Choose a reason for hiding this comment

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

so uhhhh, that is different from the existing getInterpolation and idk how to get around that
waybe we should move to dev-chat

Copy link
Contributor

Choose a reason for hiding this comment

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

having the same name is fine, maybe add a Void argument? or no, LuaValue arg? don't know which, but
i am physically unable to move to dev-chat

@Covkie
Copy link
Collaborator

Covkie commented Jul 31, 2023

Could you please list the changes made as described in pr #42

superpowers04 pushed a commit to superpowers04/Extura that referenced this pull request Sep 13, 2023
(Note, currently missing documentation)
superpowers04 added a commit to superpowers04/Extura that referenced this pull request Sep 13, 2023
(Note, currently missing documentation)
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.

3 participants