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

lua_api.txt: more detailed documentation of 'set_attach' method. #11632

Closed

Conversation

Andrey2470T
Copy link
Contributor

After some discussions on Discord and in #11582 which relate to my misunderstanding of the objects attachment mechanism, I found it necessary to finally document about it not to repeat my mistakes in future. Correct me though, if I wrote something wrong.

To do

This PR is Ready for Review.

How to test

Just read the changes in the only file.

@sfan5 sfan5 added the @ Documentation Improvements or additions to documentation label Sep 15, 2021
@sfan5
Copy link
Member

sfan5 commented Sep 19, 2021

This paragraph sounds good to me but its grammar should be fixed and the "multiply coordinate by 10" should not be called a bug.

@SmallJoker
Copy link
Member

This text would fit well into a modding guide. lua_api.txt is an API reference, not a tutorial.

@Andrey2470T
Copy link
Contributor Author

@sfan5 which a grammar should be fixed? I didn't see any mistakes or typos in the text. This is a bug because it is necessary to multiply position by 10 and also it was confirmed by rubenwardy on Discord.

@Andrey2470T
Copy link
Contributor Author

So can it be merged now or how? It is just impossible to review long-term as there is no even a code, just an extension for the API documentation.

doc/lua_api.txt Outdated Show resolved Hide resolved
@Andrey2470T
Copy link
Contributor Author

@sfan5 I made fixes by replacing some sentences with your suggested ones, but some ones I've decided to remain particularly the mention about the relative rotation is a sum of the own, the parent's and the relative rotations.

@sfan5
Copy link
Member

sfan5 commented Nov 5, 2021

That's better but grammar is still wrong in a few places, apostrophes are ' not `, the example does not make much sense (if objects are already attached their position will be exactly equal) and the reference to model dimensions is gone.

@Andrey2470T
Copy link
Contributor Author

Added references to the model dimensions in two places, replaced the apostrophes to the single quotes. I think the example is important, because I show how the relative position should be set correctly for the child. And can you elaborate in which sentences grammar is wrong?

@appgurueu
Copy link
Contributor

Should be reduced to a note on set_attach / get_attach stating (roughly) the following:

Position, rotation and scale become relative to the parent:

  • Attachment position (which must be given in model space, which is block space scaled up by a factor of 10 times parent visual scale) is added to the parent (bone) position
  • Analogeously, attachment rotation is added to parent (bone) rotation
  • Child visual scale is multiplied by parent visual scale

@Andrey2470T
Copy link
Contributor Author

Andrey2470T commented Nov 7, 2021

@appgurueu if it is better to make as notes for set_attach / get_attach, I think then it is just necessary to supplement already the existent text of those methods. The whole potential text would look like so:

  • set_attach(parent[, bone, position, rotation, forced_visible])
    • bone: string.
      • This is a name of the parent's bone that is set with set_bone_position.
      • After setting, the child will follow the movement and rotation of that bone.
      • Default is "", the root bone.
    • position: {x=num, y=num, z=num}.
      • Relative to the parent's model space.
      • Default is {x=0, y=0, z=0}.
        Note: Just like model dimensions, it should be multiplied by 10 to set it actually >proper.
    • rotation: {x=num, y=num, z=num}.
      • Rotation around each axis in degrees.
      • Default is {x=0, y=0, z=0}.
      • This rotation is added to a sum of rotations of the parent and the child.
    • forced_visible: Boolean to control whether the attached entity should appear in first >person. Default false.
      • This command may fail silently (do nothing) when it would result >in circular >attachments.

So ok?

@Andrey2470T
Copy link
Contributor Author

@appgurueu So, what do you think about my suggested changes to the attachment doc above?

@appgurueu
Copy link
Contributor

@appgurueu So, what do you think about my suggested changes to the attachment doc above?

Yes

@Andrey2470T Andrey2470T changed the title lua_api.txt: document about ObjectRefs attachment. lua_api.txt: more detailed documentation of 'set_attach' method. Jan 9, 2022
@Andrey2470T
Copy link
Contributor Author

Done.

@Andrey2470T
Copy link
Contributor Author

@appgurueu @sfan5 are your objections/suggestions about this PR? Can it be merged now?

should appear in first person. Default `false`.
* This command may fail silently (do nothing) when it would result
* `bone`: string.
* This is a name of the parent's bone that is set with `set_bone_position`.
Copy link
Member

@rubenwardy rubenwardy Apr 24, 2022

Choose a reason for hiding this comment

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

isn't it from the model/mesh?

* Relative to the parent's model space.
* Default is {x=0, y=0, z=0}.
* Note: Just like model dimensions, it should be multiplied by 10
to set it actually proper.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to set it actually proper.
compared to a world position.

* Note: Just like model dimensions, it should be multiplied by 10
to set it actually proper.
* `rotation`: {x=num, y=num, z=num}.
* Rotation around each axis in degrees.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Rotation around each axis in degrees.
* Rotation around each axis in degrees, relative to parent's rotation.

@rubenwardy rubenwardy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Apr 24, 2022
@rubenwardy rubenwardy closed this May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) @ Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants