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

Move a ton of code from player to character #39832

Closed
wants to merge 3 commits into from

Conversation

Ramza13
Copy link
Contributor

@Ramza13 Ramza13 commented Apr 23, 2020

Summary

SUMMARY: Infrastructure "Migrate more functions from player to character"

Purpose of change

Part of #34721
Moving code from player to character

Describe the solution

Describe alternatives you've considered

I intended for this to be a much smaller PR but so many things are tied together that it was impossible to move them in smaller pieces.

Testing

Additional context

int climbing_cost( const tripoint &from, const tripoint &to ) const;
void environmental_revert_effect();
/** This handles giving xp for a skill */
void practice( const skill_id &id, int amount, int cap = 99, bool suppress_warning = false );
Copy link
Member

Choose a reason for hiding this comment

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

Could I ask you to hold off on this until #39387 is either merged or closed, to prevent merge conflicts? No big deal if you don't just thought I should ask.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm so I'm scared of letting this sit around because changes made to any of the functions I moved here will be hard to spot since they are copy pasted rather than being changed in place and this is so huge. That said merge order is out of my hands anyway and this might be too big to be allowed to merge so who knows.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, yeah this is big. I can sort out the merge conflicts on my end easier than you can.

@KorGgenT
Copy link
Member

Yeah so you're right about this being gigantic. I've done stuff like this before where i went down a rathole, but you can usually get a feel for the smaller pieces that can be pulled out. I highly recommend you revisit this and cut it up into smaller chunks, and please remember: anything that invokes a uilist or a prompt should not be in Character but avatar, and do the necessary parts to fix that up if there's key logic in there.

@Ramza13 Ramza13 closed this Apr 23, 2020
@lgtm-com
Copy link

lgtm-com bot commented Apr 23, 2020

This pull request introduces 1 alert when merging dd28100 into b2532d1 - view on LGTM.com

new alerts:

  • 1 for Declaration hides parameter

@Ramza13 Ramza13 deleted the migrate_code branch November 17, 2021 17:03
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.

None yet

3 participants