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

Fix tp snipe simple #48

Merged
merged 7 commits into from
Aug 9, 2021

Conversation

oversword
Copy link
Contributor

@oversword oversword commented Aug 8, 2021

Depends on #45
Depends on #46
Depends on #47

(Attempts) to fix the TP sniping issue / the issue with unexpected behaviour
Fixes BlockySurvival/issue-tracker#83
Fixes BlockySurvival/issue-tracker#320

The fix here attempts to allow only one request per user at any time, to avoid any conflicts with multiple requests

These changes definitely need some consideration and testing, I'm unsure if they actually "fix" the problem, I have just attempted to prevent it from happening in the first place.

This the is "simple" solution, I conceived of a more complex solution based on the discussions in BlockySurvival/issue-tracker#83 , but decided to implement this one first as it was faster to write.

  • These changes may make the mod unpleasant to use, it may feel like it's constantly telling you it's not working
  • This only works if the lists are correctly cleared, something which is not guaranteed, but I may have only seen this happening in the the previous broken states
  • We might also want to send a message to the receiver telling them someone attempted to make another request so they can talk to them after they have cleared their current request

@Panquesito7 Panquesito7 added Enhancement ⚙️ New feature or request Medium priority ❕ Attention needed labels Aug 8, 2021
@Panquesito7
Copy link
Member

Panquesito7 commented Aug 8, 2021

Could you try to make a simple PR with one branch to test the changes easier and incorporate them easier (if that's fine for you), please? Thanks.

@Panquesito7 Panquesito7 added High priority ❗ Immediate attention needed and removed Medium priority ❕ Attention needed labels Aug 8, 2021
@oversword
Copy link
Contributor Author

Could you try to make a simple PR with one branch

I would really rather merge the other ones in first, I don't mind waiting for this one, the others were done to make programming this one easier

@Panquesito7 Panquesito7 removed the High priority ❗ Immediate attention needed label Aug 9, 2021
@Panquesito7
Copy link
Member

Thank you for your contributions! 😄🚀

@Panquesito7 Panquesito7 added the High priority ❗ Immediate attention needed label Aug 9, 2021
@Panquesito7 Panquesito7 merged commit 8681019 into minetest-mods:master Aug 9, 2021
@oversword
Copy link
Contributor Author

oversword commented Aug 9, 2021

@Panquesito7 Woah woah woah why did you merge this one? Are you sure this is the best fix?
I think the complex solution at least deserves considering #49

Also this one needs to be thoroughly tested, and the user experience considered, I don't think this has been properly reviewed

@Panquesito7
Copy link
Member

Should I revert this one, then? I'm kinda confused...
We can discuss this via Discord. My username is Panquesito7#3723.

Panquesito7 added a commit that referenced this pull request Aug 9, 2021
Panquesito7 added a commit that referenced this pull request Aug 9, 2021
@oversword oversword mentioned this pull request Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ⚙️ New feature or request High priority ❗ Immediate attention needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teleporting seems to be broken Fix TP snipes
2 participants