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

Ability to define and use LUA wield animations #10411

Closed
wants to merge 1 commit into from

Conversation

TheMrAmazing
Copy link

I found this old pull request: #7517
that added several wield animations in C++ that could be added to tools, such as an eating animation or a digging animation. It looks like it was rejected because you could not define new animations in LUA, making it not particularly useful for modding. I really liked the concept, so I added the ability to add new wield animations through the LUA.

This is my first time contributing, so I'm sure I've made some mistakes, but I'm happy to fix whatever is needed to get this feature added to the game

This is my second pull request for this feature, because I made a mess of the commit history on the last one

@paramat paramat added @ Script API Feature ✨ PRs that add or enhance a feature labels Sep 19, 2020
@paramat
Copy link
Contributor

paramat commented Sep 19, 2020

Thanks, i appreciate you having the patience to try again.
Because you have little experience of MT codestyle it is probably best to almost completely ignore CF, as you will not know what is acceptable codestyle. Only take notice of it if it highlights something that is very obviously an error.

@davidjohnbell
Copy link

This is a really exciting change, any news on this?

@rubenwardy
Copy link
Member

Is this ready for review?

@TheMrAmazing
Copy link
Author

Is this ready for review?

Absolutely! I'm hoping for feedback, as soon as possible because I really want to start making mods with this feature.

@paramat
Copy link
Contributor

paramat commented Oct 5, 2020

@TheMrAmazing #10468 documents how to work with ClangFormat, any comments welcome.

@appgurueu
Copy link
Contributor

appgurueu commented Jan 7, 2021

Please add documentation, fix your todos & the code style. Also resolve the merge conflict.

@appgurueu
Copy link
Contributor

Related: #10360 also allowing for custom wield animations

@TheMrAmazing
Copy link
Author

Please add documentation, fix your todos & the code style. Also resolve the merge conflict.

Is this still something I should bother with, it has been months and very little interest seems to have been shown?

@appgurueu
Copy link
Contributor

Please add documentation, fix your todos & the code style. Also resolve the merge conflict.

Is this still something I should bother with, it has been months and very little interest seems to have been shown?

I would sure like to use this feature. Fixing the merge conflict, code style & adding documentation is not that much work. I don't know about the TODOs, I only skimmed them.

I would not like to see those seven hundred lines of code go to waste.

@ZedekThePlagueDoctor
Copy link

I too would like to see this implemented. Viewmodels / wielding could use a lot of work and extension in general.

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Feb 26, 2021
@sfan5
Copy link
Member

sfan5 commented Feb 26, 2021

Here's a list of problems I identified:

  • Code style
  • Lack of documentation
  • unused functions/global variables, debug-looking code still lying around, code commented out instead of deleted
  • repository of wield animations is just a global variable, no transfer from server to client. This doesn't work. (the serialization code seems to exist already?!)
  • read_wield_animation should be side-effect free (not handle repository init and insertion, only read and return the animation)
  • read_wield_animation has some code duplication, also not sure if the vector + sort is necessary (lua tables should already be sorted)
  • code modernization in splinesequence especially manual iterator usage

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 26, 2021
@rubenwardy rubenwardy closed this Oct 31, 2021
@rubenwardy
Copy link
Member

Closed due to lack of response

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) Feature ✨ PRs that add or enhance a feature Possible close Rebase needed The PR needs to be rebased by its author. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants