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

Make theifs no longer have objectives for animals who dont exist #259

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ermucat
Copy link

@Ermucat Ermucat commented Dec 30, 2024

About the PR

Changed VerifyMapExistence from false to true so now the game checks if the animal exists before you try and steal it, also this works with group animals like StealIan group

Why / Balance

Stops weird bugs in the future and make sure that people dont get animals that dont exist, also for late-join theifs whos animals are gone

Technical details

Changed VerifyMapExistence Component from false to true

Media

image

Requirements

Breaking changes

Changelog

🆑

  • fix: Theifs no longer have to steal animals that dont exist

@FluffMe
Copy link
Collaborator

FluffMe commented Dec 30, 2024

This sounds like something that should be in upstream

Copy link
Collaborator

@FluffMe FluffMe left a comment

Choose a reason for hiding this comment

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

Looks legit.
However, i have a question:

  • If that's such a simple fix, why isn't this a wizden PR.
  • If wizden did this on purpose, do we want this?

Slapping admin input on this. Still looks like upstream PR to me.

@FluffMe FluffMe added S: Awaiting Admin Input Discussion by the admin team is required DO NOT MERGE S: Approved by Maintainer and removed S: Needs Review Review is requested labels Dec 30, 2024
@spanky-spanky
Copy link
Collaborator

spanky-spanky commented Dec 30, 2024

Slapping admin input on this. Still looks like upstream PR to me.

Should be PR'd upstream in theory, but from an admin perspective, this is a no brainer. This is a recurring, albeit uncommon, ahelp issue.

@spanky-spanky spanky-spanky added S: Approved by Admin Team and removed S: Awaiting Admin Input Discussion by the admin team is required labels Dec 30, 2024
@FluffMe
Copy link
Collaborator

FluffMe commented Dec 30, 2024

@Ermucat do you want to do this upstream instead? Or just harmoy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants