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.

Sign up for GitHub

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

Jump to bottom

Core: Gardening updates from Eden #6072

Closed
wants to merge 1 commit into from
Closed

Core: Gardening updates from Eden #6072

wants to merge 1 commit into from

Conversation

zach2good
Copy link
Contributor

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • [not yet] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

I was poking around open PRs and I saw we have some stuff about gardening, so I went and did some comparisons with Eden. I clarified some comments and took what changes I noticed by eye

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 4, 2024

✨ Thanks for the PR! ✨

This is a friendly automated reminder that the maintainers won't look at your PR until you've properly completed all of the checkboxes in the pre-filled template.

@zach2good @jmcmorris @59blargedy
Core: Gardening updates from Eden
7d353d4 
Co-Authored-By: Jasson McMorris <[email protected]>
Co-Authored-By: 59blargedy <[email protected]>
@zach2good zach2good force-pushed the gardening_tweaks_from_eden branch from 0bd4dae to 7d353d4 Compare August 4, 2024 14:40
lapislosh
lapislosh reviewed Aug 5, 2024
View reviewed changes
src/map/utils/gardenutils.cpp
@@ -275,7 +278,14 @@ namespace gardenutils
strength += dominantAura / 10;
}

strength += (int16)((100 - strength) * (PItem->getStrength() / 32.0f));
strength += (int16)((100 - strength) * (PItem->getStrength() / 31.0f));
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a previous PR that fixes this a different way: #4522
I chose to increase the RNG to 0-32 and keep the divisor as 32, I guess they chose to drop the divisor to 31 but leave the range from 0-31. Either is probably fine tbh but we can't have both changes together or it'll be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for weighing in, I was just pulling things over willy-nilly 👍

@lapislosh
Copy link
Contributor

Also for completeness sake, I had removed double ore harvests from my previous PR with the comment:

If we did want to keep the maximum as 2 but make it super rare, we would need to adjust the gardening_results.sql to have multiple entries for each ore, and only the last entry would have a yield of 2.

This PR makes this change, and so double-ore harvests will now be possible again (and it's done the correct way here with 2 separate entries to make it rare, instead of having one entry with a range of 1-2).

@zach2good zach2good added the hold On hold, pending further action/info label Aug 5, 2024
@zach2good
Copy link
Contributor Author

I've abandoned this PR. Downstream has apparently been testing and trying to fix some of the small bugs in gardening, so hopefully they'll eventually find their way back upstream one day

@zach2good zach2good closed this Nov 18, 2024
@zach2good zach2good deleted the gardening_tweaks_from_eden branch November 18, 2024 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lapislosh lapislosh lapislosh left review comments

Assignees
No one assigned
Labels
hold On hold, pending further action/info
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zach2good @lapislosh