The Wayback Machine - https://web.archive.org/web/20200928031730/https://github.com/cuberite/cuberite/pull/4802
Skip to content

/ cuberite

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 correct implementation of crops #4802

Open
wants to merge 14 commits into
base: master
from
Open

Add correct implementation of crops #4802

wants to merge 14 commits into from

Conversation

@0ddlyoko
Copy link

0ddlyoko commented Aug 2, 2020 •
edited

This Pull Request fix some bugs linked to crops.

Some changes are:

  • Crops now drops like vanilla minecraft (See Melon, Pumpkin, Carrots, Potatoes & Wheat, Wheat
  • Fortune works on crops
  • Add method IsFullGrownPlantAt(Vector3i) in classes cChunk, cChunkMap, cWorld
  • Add method IsFullGrown(cChunk &, Vector3i) in class cBlockHandler
  • These methods will check if crop at a given location is full grown
  • No longer create a melon / pumpkin when you right-click with a bone meal on a full grown melon / pumpkin seed
  • Fixes #4801
  • Fixes #4803
  • Fixes #4805

Futur (tomorrow)
0ddlyoko added 7 commits Aug 2, 2020
@0ddlyoko
[FIX] Add correct implementation of seed drops.
Loading status checks…
fb9da2b 
> Official percentage of drops has been implemented
@0ddlyoko
Fix C++ conventions
Loading status checks…
09ad081
@0ddlyoko
Change "Vals" variable to "m_Vals"
Loading status checks…
0bd8d36
@0ddlyoko
[FIX] Add correct implementation of Carrots, Potatoes, Wheat & Beetro…
Loading status checks…
a2e0288 
…ots seed
@0ddlyoko
Fix incorrect values
Loading status checks…
6bf8745 
- Carrots: 1 - 5
- Seeds: 1 - 4
@0ddlyoko
Add Fortune support with crops
Loading status checks…
0347f56 
Add fortune support with Wheat, Carrots, Potatoes & Beetroots seeds
@0ddlyoko
[FIX] Right-clicking on a grown Beetroot in survival consume 2 bone m…
Loading status checks…
ade6ade 
…eals

Fix #4805
@0ddlyoko 0ddlyoko changed the title [WIP] Add correct implementation of seed drops. [WIP] Add correct implementation of crops Aug 2, 2020
0ddlyoko added 4 commits Aug 3, 2020
@0ddlyoko
Add documentation for "cWorld::IsFullGrownPlantAt" method
Loading status checks…
96b7821
@0ddlyoko
Fix dispenser that full grown a plant
Loading status checks…
c799b84 
> Change methods cItemDyeHandler::FertilizePlant & cItemDyeHandler::growPlantsAround to static
@0ddlyoko
Display particle even if tree doesn't grow
Loading status checks…
2fc3de8
@0ddlyoko
When right-clicking on a full grown melon / pumpkin seed, no longer p…
Loading status checks…
1689ecd 
…roduce a melon / pumpkin

Before this commit, when you right-click on a melon or a pumpkin seed, a melon / pumpkin block spawned.
With this commit, it no longer spawns
@0ddlyoko 0ddlyoko marked this pull request as ready for review Aug 5, 2020
@0ddlyoko 0ddlyoko changed the title [WIP] Add correct implementation of crops Add correct implementation of crops Aug 5, 2020
@0ddlyoko
Copy link
Author

0ddlyoko commented Aug 5, 2020

@tigerw
I think I'm ok with this PR, could you check it and say me if there is any problem with it or any improvement ?
@bearbin
bearbin reviewed Aug 5, 2020
View changes
src/Blocks/BlockCrops.h Outdated Show resolved Hide resolved
src/Blocks/BlockCrops.h Outdated Show resolved Hide resolved
src/Blocks/BlockStems.h Show resolved Hide resolved
0ddlyoko added 3 commits Aug 9, 2020
@0ddlyoko
Revert commit "Add correct implementation of crops"
7c09806 
This commit revert commit "Add correct implementation of crops" (1689ecd) as said bearbin (#4802 (comment))
@0ddlyoko
[FIX] Remove useless TODO & import
Loading status checks…
609a91b
@0ddlyoko
[FIX] Do not create melon / pumpkin block when right-clicking with a …
Loading status checks…
6afaff3 
…bone meal

This fix will prevent the creation of a melon / pumpkin block when you right-click with a bone meal on a melon / pumpkin plant
- It just detect if the plant is full grown. if yes, the method "Grow" is not called
@0ddlyoko
Copy link
Author

0ddlyoko commented Aug 9, 2020

Done @bearbin
@bearbin
bearbin requested changes Aug 9, 2020
View changes
Copy link
Member

bearbin left a comment

Idea is good but this code will not actually work correctly.
src/Blocks/BlockStems.h
@@ -60,18 +85,28 @@ class cBlockStemsHandler:
auto oldMeta = a_Chunk.GetMeta(a_RelPos);
auto meta = oldMeta + a_NumStages;
a_Chunk.SetBlock(a_RelPos, m_BlockType, static_cast<NIBBLETYPE>(std::min(meta, 7))); // Update the stem
if (meta > 7)
if (oldMeta == 7)

This comment has been minimized.

Sign in to view
@bearbin

bearbin Aug 9, 2020 Member

This change is still wrong. The Grow function was fine as it was.

This comment has been minimized.

Sign in to view
@0ddlyoko

0ddlyoko Aug 10, 2020 Author

I don't really understand what can I do if I don't have to edit this method, here I've tested this method and it works

This comment has been minimized.

Sign in to view
@bearbin

bearbin Aug 12, 2020 Member

I'll rework this myself and then merge - but can you explain what your reason was for changing this function in the first place?

This comment has been minimized.

Sign in to view
@0ddlyoko

0ddlyoko Aug 12, 2020 Author

With the old implementation, when you right-click on a melon / pumpkin seed with level = 6, the level of the plant become 7 (full grown) and a melon / pumpkin block appears.
This is because this method checks when level will become > 7.
With this implementation, if old level is before 7 (so 6, 5, ...), no melon / pumpkin block will spawn.

This comment has been minimized.

Sign in to view
@0ddlyoko

0ddlyoko Aug 12, 2020 Author

Another way is to create a special method when a player / dispenser want to grow melon / pumpkin or add a new parameter to this function, but after some tests it seams this solution works
src/Items/ItemDye.h
@@ -118,6 +118,11 @@ class cItemDyeHandler :
{
// Grow by 2 - 5 stages:
auto numStages = GetRandomProvider().RandInt(2, 5);
// No longer create a melon / pumpkin block when right-clicking on a grown melon / pumpkin seed

This comment has been minimized.

Sign in to view
@bearbin

bearbin Aug 9, 2020 Member

We should not do nothing here, we only need to cap the numStages in the call to GrowPlantAt to be min(7-currentGrowth, numStages)

This comment has been minimized.

Sign in to view
@0ddlyoko

0ddlyoko Aug 12, 2020 Author

This is to prevent creating a melon / pumpkin block when someone right-click on a full grown plant with bone meal.
This prevents also dispenser to do it.
@tigerw
Copy link
Member

tigerw commented Aug 12, 2020

To summarise (please correct any mistakes): our current issue is that bonemeal causes crops both to grow stages, and spawn yummy melon. However, Vanilla doesn't spawn melon when using bonemeal; the crop only advances stages, up to a maximum of 7.

In this case, as you said, I think we should define Grow as only making the stem higher. Then, add a new function in cBlockPlant, BearFruit, that is called when a block tick happens in OnUpdate. cBlockStemsHandler then overrides BearFruit to spawn melon, which now only happens during a random tick.

This also has the advantage of not needing to add IsFullyGrown to cWorld (we're trying to reduce cWorld's size) xD

So something like:

// Plant

cBlockPlantHandler::OnUpdate()
{
  if (Grow() == 0)
  {
     // Growth at max, spawn any fruit (pumpkin, melons):
     BearFruit();
  }
}

// Stems

cBlockStemsHandler::Grow(const int Stages)
{
  const auto Meta = std::min(7, OldMeta + Stages);
  SetMeta(Meta);
  return Meta - OldMeta;
}

cBlockStemsHandler::BearFruit()
{
  // Whatever growProduce contained
}

D'you think this will work?
@bearbin
Copy link
Member

bearbin commented Aug 12, 2020

Tigerw's suggestion seems reasonable, and also has the advantage of being extensible to other new types of fruit that might be added.
@0ddlyoko
Copy link
Author

0ddlyoko commented Aug 12, 2020

Same for me, I'll implement your solution when I have time
@KingCol13 KingCol13 mentioned this pull request Sep 19, 2020
Merged
@tigerw
Copy link
Member

tigerw commented Sep 23, 2020

Hey @0ddlyoko are you currently working on this? If you're busy I could apply the changes and merge.
@KingCol13
Copy link
Contributor

KingCol13 commented Sep 23, 2020

@tigerw I've got the fortune stuff implemented in a branch ready to pull after I rebase it on top of your style fixes.
@0ddlyoko
Copy link
Author

0ddlyoko commented Sep 23, 2020

Hello @tigerw I'm not working on it right now, you can merge if you want or you can apply changes & merge
@tigerw
Copy link
Member

tigerw commented Sep 23, 2020

Thanks all!
@KingCol13 KingCol13 mentioned this pull request Sep 23, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bearbin bearbin

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

[BUG] Right-clicking with a bone meal on a full grown BeetRoot in Survival consume 2 bone meals
[BUG] Bone Meal in Dispenser changes crops from age 0 to age 7
[BUG] Crops drops aren't like official implementation
4 participants
@0ddlyoko @tigerw @bearbin @KingCol13
You can’t perform that action at this time.