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

#4536 close parcel send thru mail system to other player #4537

Closed
wants to merge 1 commit into from

Conversation

gesior
Copy link
Contributor

@gesior gesior commented Sep 19, 2023

Auto close parcel, when it's send to other player thru mail system.

@EPuncker EPuncker added bugfix priority: critical Issues with this label should be resolved as quickly as possible labels Sep 19, 2023
@EPuncker EPuncker linked an issue Sep 19, 2023 that may be closed by this pull request
@luanluciano93
Copy link
Contributor

The solution referenced in issue #4535 is better, but you ignore it because it is a canary.

@joseluis2g
Copy link
Contributor

The solution referenced in issue #4535 is better, but you ignore it because it is a canary.

so if you really think that solution is somehow "better", would you be able to elaborate how that PR is better?

@gesior
Copy link
Contributor Author

gesior commented Sep 22, 2023

The solution referenced in issue #4535 is better, but you ignore it because it is a canary.

I would call it hotfix. Like Gunzodus's "block possibility to send parcel, when you are standing 1 sqm from mailbox". It fixed crash issue, but not solved real problem.

Canary fix does not solve all possible scenarios [ex. moving container into player Inbox from Lua - without 'mail system'] and add new bug to engine: close parcel when you put it on mailbox, even if it's not delivered to player [ex. it's addressed to player that does not exist].

Of course I've ignored it. I don't watch canary commits. They are all the same: fix part of problem + add new problem.

@EPuncker
Copy link
Contributor

just a note in case anyone is wondering on why this PR has not been merged yet, it was targeted specifically to branch 1.4, which currently doesn't have any active maintainer thus we still haven't decided how to handle this as of yet

if anyone is feeling like doing so, open a new PR targeting master branch and adding the due credits and we will merge it as soon as possible

@Zbizu
Copy link
Contributor

Zbizu commented Sep 27, 2023

The solution referenced in issue #4535 is better, but you ignore it because it is a canary.

Canary solution is fetching tiles and spectators again and doing at least two extra loops (getSpectators and "for" spectators) when all that had to be done was to add one "if" statement in place that already had all required variables and was already part of the loop that is going through the container spectators.

in short: canary solution has two unnecessary loops

@ranisalt ranisalt closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix priority: critical Issues with this label should be resolved as quickly as possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mailbox steal depot items bug, crash OTS bug [clone items]
8 participants