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

Add following through wormholes support #16

Merged
merged 7 commits into from
Dec 10, 2021
Merged

Conversation

sauktux
Copy link
Member

@sauktux sauktux commented Nov 26, 2021

Closes #3.

@sauktux sauktux linked an issue Nov 26, 2021 that may be closed by this pull request
3 tasks
@sauktux
Copy link
Member Author

sauktux commented Nov 26, 2021

I was thinking about pausing the follow thread while a seperate thread(The wormhole travel thread) does its thing, however, there is no pause functionality. As such, I had to just to leave it in a state of looping and checking.
Do you believe a better solution would be to stop the following thread completely and then attempt to start it again once the player has travelled through the wormhole?

@victorpopkov
Copy link
Member

Ideally, we should change the behaviour of the current threads from less direct usage to the corresponding functionality in entities themselves, like entity tasks as Klei have originally intended. But at some point, I've just decided to leave everything as-is for some reason.

When it comes to your solution, I think everything should be fine if it works. I'll check later as soon as I will overcome my unwillingness to open the game.

@victorpopkov victorpopkov added the enhancement New feature or request label Nov 26, 2021
@victorpopkov victorpopkov added this to the Release v0.22.0 milestone Nov 26, 2021
@victorpopkov
Copy link
Member

victorpopkov commented Nov 28, 2021

Force-pushed to:

  • Rebase
  • Resolve conflicts (just accepted your variant)

Copy link
Member

@victorpopkov victorpopkov left a comment

Choose a reason for hiding this comment

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

You've mentioned in Slack about the "nitpicks" and have asked me to comment. So I saw this as a sign that you want to try fixing everything yourself, so I'll just review instead.

The only thing I've done: rebase to the latest develop, so that you could've CI to assist you. I've also resolved all the conflicts by just accepting your variant. Keep in mind, that your variant is outdated, as it doesn't include the merged #8.

When it comes to the business logic, I didn't test this feature yet, and I believe you are more qualified in this aspect than I am since you are the one who is implementing it. I trust you, so use whatever approach you think fits best.

.luacheckrc Outdated Show resolved Hide resolved
scripts/components/keepfollowing.lua Outdated Show resolved Hide resolved
scripts/components/keepfollowing.lua Outdated Show resolved Hide resolved
scripts/components/keepfollowing.lua Outdated Show resolved Hide resolved
scripts/components/keepfollowing.lua Outdated Show resolved Hide resolved
scripts/components/keepfollowing.lua Outdated Show resolved Hide resolved
@sauktux
Copy link
Member Author

sauktux commented Nov 28, 2021

Force-pushed to squash several commits, which are doing the same thing: fixing StyLua and Luacheck issues.

@victorpopkov
Copy link
Member

Force-pushed to:

  • Add LDoc comments
  • Apply code style guidelines since the beginning
  • Squash some commits

Copy link
Member

@victorpopkov victorpopkov left a comment

Choose a reason for hiding this comment

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

Well done! Everything works from my side. Have tested on dedicated server with 2 real players.

@victorpopkov victorpopkov merged commit b239f2f into develop Dec 10, 2021
@victorpopkov victorpopkov deleted the sauktux/feature-3 branch December 10, 2021 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add following through wormholes support
2 participants