Fix cable plate placing on several-blocks nodes #358
Conversation
Athozus
changed the title
|
edit. never mind, you changed this already and those tests are passing. However, does it need other than normalization of fine_pointed vector (or pointed_thing.above)? That would be cleaner as there's |
|
Basically what you're doing is normalization afterwards. Normalizing beforehand is just another way to do the same. But I guess, whether going with current way for normalization or using vector.normalize, it probably would be good to also add comment that explains corner case why it has to be normalized. |
|
Did read whole thing again and to handle all cases wouldn't it be required to normalize way before current solution, on line 99: local pointed_thing_diff = vector.subtract(pointed_thing.above, pointed_thing.under)Change to local pointed_thing_diff = vector.subtract(pointed_thing.above, pointed_thing.under):normalize()Wouldn't that make sure it is handled in all possible corner cases, make it bit more cleaner / understandable and also shorter? edit. Actually there's even better shorthand method for it (documented since 5.0.0): local pointed_thing_diff = vector.direction(pointed_thing.above, pointed_thing.under)
|
Replacing |
Checked this, try with: local pointed_thing_diff = vector.direction(pointed_thing.under, pointed_thing.above)and fine_pointed = vector.direction(pointed_thing.above, fine_pointed)Notice that I've swapped arguments. I'm not sure however if this is correct or not, didn't really think it through and it is likely that directions have been messed up either since beginning or since I've rewritten this thing for cable plate placement. |
Still not very precise. I'm currently decomposing the algorithm because I think it can be 50% shorter. EDIT : My bad, the fix was working ; I had just changed some things while debugging. However, I still think that it could be shorter. |
|
Imho that could be something like this : local pointed_thing_diff = vector.direction(pointed_thing.under, pointed_thing.above)
local dir = math.abs(pointed_thing_diff.x + 2*pointed_thing_diff.y + 3*pointed_thing_diff.z) -- 1, 2 or 3
local num = dir + 3/2*(1-pointed_thing_diff[dir]) -- dir + additional negative offset
local node = {name = nodename.."_"..(num ~= 0 and num or 1)}
return item_place_override_node(itemstack, placer, pointed_thing, node)EDIT : It works without Sneak, it doesn't with. 👺 |
aux1 placement grid for which fine_pointed is used is like this: So there's about 5 * 6 different placements around node when using aux1 and total 36 different placement options. However, it also seems that pointed_thing can sometimes be diagonal with oversized nodes in which case resulting direction will be float even while positions are not. Currently this does fix some placements but not all, those that this wont fix also can't be fixed with original deferred normalization (which in turn only fixes cases where aux1 is held, that is nodes with formspec or other node specific action). Normal placement, even while broken because of this, shouldn't however cause crash as it will just try to place a node that does not exist. So I'd recommend this approach as a base as it, at least in theory, should get few things right that were wrong before but it still needs some additional work. Not sure if we could round/floor here, it isn't really that clear where plate should be placed when pointed_thing is diagonal. |
Yes I noticed that one with some chat-debugging, so I'm with my paper (and
I tested with some |
param2 is very complicated, wouldn't recommend bringing that here. Nodes do not follow conventions for this.
Because of limitations of connected nodeboxes, those can't be rotated but are always static. |
|
@S-S-X note that it can be merged, the thing is fixed. The PR with a more simplistic determination of the pointed face would come later. |
|
Could maybe use a raycast to get the pointed position, that's what I used for |
The thing isn't fixed. The thing will crash in certain cases. This does anyway fix some cases and prevents some crashes, but only when pointed thing under isn't diagonal to above node. If merging this fast to fix crashes then I'd suggest adding check for diagonal pointed thing and just cancel placement before even trying to do anything. Then we could also close linked issue, atow this PR isn't really enough to close linked issue.
Not sure how this is supposed to change it, placement already does ray cast and we do already have node position. What we do not have is direction in remaining few corner cases. |
|
@Athozus could just use something like this to cancel action for remaining corner cases, allow at most single axis but do not care how far it is: def.on_place = function(itemstack, placer, pointed_thing)
count = 0
for axis in pairs(xyz) do
count = count + (pointed_thing.under[axis] == pointed_thing.above[axis] and 0 or 1)
if count > 1 then
return itemstack
end
endCould maybe add comment too with link to this PR |
This code works fine (I kept after that pre-condition the code I wrote upper). Maybe we should just returner the
What do you exactly mean ? Should I add : ? -- related to github.com/mt-mods/technic/pull/358 |
I think most correct way is to just return input stack unchanged, we do not want to start playing with item counts.
Yeah, I think that could be nice to have just above loop or somewhere near it so that reason for this check becomes more obvious. I mean count loop and return seems bit weird at first. It is true one could check git blame and find this discussion but sometimes it is better to just add comment. |
Mineunit failed regression tests, click for detailsRegression test log for Technic CNC:Regression test log for Technic Chests:Regression test log for Technic: |
technic/machines/register/cables.lua
Outdated
| local pointed_thing_diff = vector.direction(pointed_thing.above, pointed_thing.under) | ||
| local dir = math.abs(pointed_thing_diff.x + 2*pointed_thing_diff.y + 3*pointed_thing_diff.z) -- 1, 2 or 3 | ||
| local num = dir + 3/2*(1+pointed_thing_diff[dir]) -- dir + additional offset |
There was a problem hiding this comment.
What was this supposed to do and how exactly does it handle placement options?
For now it does fail significant portion of test cases and instead of fixing bug it seems to be an attempt to rework whole user facing placement behavior.
There was a problem hiding this comment.
pointed_thing_diff: you know ;)dir: the previously index, but reinforced- x = +/- 1 returns 1
- y = +/- 1 returns 2
- z = +/-1 returns 3
num: the number of thecable_plate_nummodel ; it's working likey = ax+bdir +: because its keep thatbwhich means the axis1+pointed_thing_diff[dir]:- returns 0 for x/y/z = -1
- returns 2 for x/y/z = 1
*3/2: add this offset of 0 or 2, depending on the value
This is relatively short, and understandable, according to me.
There was a problem hiding this comment.
It is but it is reworking whole placement behavior which in my opinion is not good thing to do.
Needs some more discussion about before going there. Bugs can be fixed without changing behavior, for that I'd of course give approval but for changing behavior without good enough reasoning reject from me.
There was a problem hiding this comment.
If going to make placement behavior completely different from what it was:
Update description and title, update tests and provide reasoning why new behavior is better for players than previous one.
If going to just fix crash then revert to original placement behavior but keep fixes (diagonal checks + vector.direction on 2 lines).
Athozus
force-pushed
the
nil-cable-plate
branch
from
ecf55a6
to
90a207d
Compare
|
Reverted to previous commit (work on simplification attempt potentially for later). |
This does still need that diagonal placement check, was also dropped while reverting: #358 (comment) |
There was a problem hiding this comment.
Looks good now, should fix placement crash for all over sized support nodes.
- In some cases placement work correctly and probably how most would expect it to work.
- In some cases placement is prevented to not crash it, some of these could have valid position for placement but are very few special cases so it is better just skip placement for now.
These changes were tested a bit too.
Athozus
force-pushed
the
nil-cable-plate
branch
from
24041a4
to
9cf55fd
Compare
|
I rebased it, changes look fine. Ofc for this one a squash 😅 @S-S-X I let you do the action, if you have anything to add/change. |
Co-authored-by: SX <[email protected]>
Co-authored-by: SX <[email protected]>
Click for detailed source code test coverage reportTest coverage report for Technic CNC 87.39% in 11/14 files:Test coverage report for technic chests 45.58% in 6/6 files:Test coverage report for technic 66.93% in 97/97 files:Raw test runner output for geeks:CNC: Chests: Technic: |
|
Another PR was merged so rebased again, most testing I did was exactly for these changes / current state and there's multiple automated tests confirming that most placements work like before. Should be good enough so merging this. |
The description is not very well because it's hard to describe what I've done in a single, notably in English. I can force-push to a better commit message if needed. EDIT : I think the title describe better the changes now.
What I've done :
minetest.chat_send_allthat theindexvalue was exceeding the range of #xyz, so it causednilindex infine_pointedcall.indexvalue within the range of 3 divisors. Lua starts numbering at1, so it does that weird(index-1)%3+1Fix #305