Use uncarved pumpkin in survival #2119
No reviewers
kabou
Labels
No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
feature request
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: VoxeLibre/VoxeLibre#2119
Loading…
Reference in New Issue
No description provided.
Delete Branch "uncarved_pumpkins"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This has just recently been merged in mineclonia and I seem to be getting better at merge conflict resolution ^^.
As mcla does this has been tested ad nauseam here Mineclonia/Mineclonia#285 - refer to that PR for very detailed testing instructions. tl;dr the gourd behavior is supposed to be perfect now - didn't test that, don't care a whole lot tbh.
Please just test this:
Did you forget a commit or was the pumpkin shearing sound location code already fixed in MineClone2?
no idea. but yeah it said working tree clean on one commit.
IIRC that bug led to a warning being logged, so it makes sense someone fixed it already.
Tested a bit: uncarved pumpkins grow naturally and when farmed. Can be carved (gives seeds) and worn. Gourd disconnects when carved.
Looks good.
Please test all the other interactions I listed in the original PR too. This thing looks extremely simple, but it actually touches a lot.
For example, the solution I tried originally led to old pumpkin stalks no longer producing pumpkins. It is important you test the code with a world where you grow pumpkins, then load it with this branch and see if the farm is broken.
Nice! but the top texture rotates when you carve it...
"erlehmann":
As a reminder, an even smaller amount of testing was requested by the PR submitter:
"cora":
"erlehmann":
I might revisit my testing later on, if @cora states that the differences between mineclonia's and mineclone2's version of the code warrants it. I got the initial impression that she did not think that that is the case.
Well, do not complain to me if it is broken – go complain to a mirror in that case.
Merge conflict resolution is something that can introduce breakage. I do not know how much needed to be resolved … but in the absence of automated tests, I think that just doing all the things that were added or changed once is a very low bar for approval …
I am very confident that this code works in Mineclonia. I do not know where cora gets the same confidence that it works in MineClone2. If all commits had applied cleanly, I might have agreed that further testing is a waste of time. But any time a merge conflict happens, some patch is applied to code that is different than the one it was originally developed against. So that alone is a good reason to be vigilant here.
@kabou nvm I saw that you approved it already. I guess you read the code and liked it well enough. Do you have any comments on the way I solved this? Would you have done it differently? If so, how?
Other than a quick glance at the changes and a luacheck run, no I did not perform a code review to a depth that you seem to like to see done. Already I pointed out why I tested what I tested and the grounds for my approval were also stated clearly.
If you don't agree with this PR, you are at liberty to do the review that you like to see and withold your approval until it meets your standards. If you don't like my review, do a better one yourself. What are your priorities here, reviewing the actual PR or reviewing only the PR's reviews?
You know LOTR? It's a story about an emergent technological civilization that gets crushed by a group of rather old people (wizards and elves) because they wanted to turn the earth into a cheap copy of their magical homeland^^While i didn't test all of the instructions from mcla, I did test a bit more than I requested. But like i said even in the unlikely case that merging broke something (they were really trivial conflicts) the basic pumpkin stuff works. I will try golems again later bc i think i did not yet.
I think it's nice that you put all this work into it and from what I read this is really perfect now. But please accept that this isn't mineclonia @erlehmann. I have made the decision that no further testing is required (other than basic functionality) because I have made some random tests after merging and everything looks perfect just as your code usually is.
I know there's the chance something will slip in this way but I am willing to take that in favor of less testing overhead. The same strategy will work if we need to fix something about it (which we both know won't happen ;) ).
497dc848eeto67ae203772rebased
In any case I think it is said not often enough: Thank you @erlehmann for the quality of your commits. Because they are so good (clean) they usually cleanly apply or only, like in this case trivial things need to be resolved and this isn't a small PR.
And either way, thank you for caring even if we won't turn mcl2 into magical homeland <3.
it does ... i think that's acceptable ^^
Tested some more things from the list. P sure this works. I'm merging it :)
I wanted an opinion on my code from you. I will probably not ask again for that.
That's funny, but it's the same in Minecraft. I didn't mention that particular thing because why would we care about the stem moving like in Minecraft? :P It was even reported as a bug and they closed it as "Won't fix."
Small issue: you can't carve the pumpkin from the top side. The carved face can still be determined by the player's position relative to the pumpkin. And if the angle is a perfect 45 degrees, pick a side by default. By the way, carving should also work from the bottom.
Apologize if I hurt your feelings. Please also understand that telling me to "shut up" in one topic and then the next minute expecting me to do in-depth code reviews for you while I'm also reviewing other PR's and trying to do other stuff does not help the communication. Maybe we should both turn the snarkiness knob down a bit on occasion. Sometimes.
LMAO no.
I only expected that whoever approves code I ahve written may have read it.
You often have ideas about how to do a thing differently, therefore I asked.
What's so funny? That's exactly how it's done in Minecraft - tested personally.
Ok, two issues with the endermen.
Wearing the carved pumpkin should not make them ignore you if you attack them - they're supposed to become aggressive.
In the current implementation, a triggered enderman will "forget" the player once they wear the carved pumpkin. That shouldn't be the case - once triggered, they should aggressively follow the player.
Good luck implementing that – I am curious how you plan to achieve such a thing.
Does this PR introduce those problems?
Look, I did have a glance at the code to probe the extent of the changes. The changes were limited and looked okay. In combination with cora's reassurances that this code from mineclonia only needed minimal checking, I reserved most of my PR review efforts to actual play testing the code.
So I did read the code, albeit cursorily. I did not make a thorough study of the code paths in mcl_farming that were affected by the changes, as a true in-depth review would have required.
You have to consider people's motivation for doing mineclone-related work. Sometimes when I feel like learning more about existing code and also have time and enery to spare, then I'll take a PR as an opportunity to delve into the broader area of code affected. In this situation, I did not have a lot of time to spend, but did notice that cora had been doing quite a bit work without it receiving much attention from others. So I felt like I should at least make an effort to keep the PR's moving.
I didn't address that issue to you. So just because you have no idea how that could be achieved, doesn't mean no one can do it. Check your attitude, you're acting weird again.
I don't know how it was before this PR, I only mentioned it here. I'll open a separate issue if @cora says it's unrelated.
Well I didn't test but I'd suspect that to be in the enderman code. I'll have a loom later maybe fix it in the mobs pr
Ok, I'll open a separate issue then, so it doesn't slip through the cracks.
Have you noticed the other issue about carving the pumpkin from the top and bottom sides? I have a feeling it's not hard to solve. After all we check angles when placing stairs, so it's definitely doable, just not sure how the code would look like.