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

Fix MC-868 #9709

Closed
wants to merge 2 commits into from
Closed

Fix MC-868 #9709

wants to merge 2 commits into from

Conversation

TreemanKing
Copy link
Contributor

@TreemanKing TreemanKing commented Sep 11, 2023

Fixes MC-868
Adds a check if distance between minecart and detector rail is less than maximum distance of the centre of a block to the edge of a block (0.5)

Closes
#9698

Fixes MC-868
Adds a check if distance between minecart and detector rail is less than maximum distance of the centre of a blcok to the edge of a block (0.5)
@TreemanKing TreemanKing requested a review from a team as a code owner September 11, 2023 09:11
@vadcx
Copy link

vadcx commented Sep 11, 2023

Throughout the MC codebase to avoid the math.sqrt function (expensive) the distance checks are made on squared values.

If the classic formula is math.sqrt((x2-x1)²+(y2-y1)²) < limit

then removing sqrt on both sides of the equation leaves (x2-x1)²+(y2-y1)² < limit²

The ...distanceSquared function should be somewhere in vanilla. Please double-check me on the 0.5d² value.

Copy link
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

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

This breaks (possibly) expected behavior for the detector rail as it currently operates. I created the setup below and summoned a minecart at varying points on the track horizontally. In vanilla, I could spawn it on 3/10ths of the adjacent block and it would still trigger the detector rail. With this PR, the position has to be within the detector block x/z coord.

@TreemanKing
Copy link
Contributor Author

TreemanKing commented Sep 14, 2023

This breaks (possibly) expected behavior for the detector rail as it currently operates. I created the setup below and summoned a minecart at varying points on the track horizontally. In vanilla, I could spawn it on 3/10ths of the adjacent block and it would still trigger the detector rail. With this PR, the position has to be within the detector block x/z coord.

From testing that I did prior to posting the PR, I used the sqrt(2) as the maximum distance (centre to corner). When I had used this, the bug would still persist (considering a velocity of 0.7 or greater from memory), which I believe what current basic vanilla function uses. I think this is because the block is picked up by the detector rail on the corner is made by the rail converting the rail.

I believe the vanilla state is broken as a result as the block should only take the entities from the basic orthogonal directions as it can only face 4 directions, and to this that it's attempting to detect then interacting entity (entity within block), not centre to corner.

I do believe some redstone contraptions that use detector rails should be used as a test to see if any current redstone contraptions will break. Only thing I can think of is if the redstone is required to be on for that fraction of a second longer or a block as you mentioned.

@vadcx
Copy link

vadcx commented Sep 17, 2023

Treeman, did you consider the fix posted in the originial MC-868? Link to comment by FX - PR0CESS

Now I will mention why this should not be fixed xD

When changing this code you run into an issue with client rendering. Due to the client using interpolation, the client will render the minecart as if it's turning and then put it in the right place. So that issue should be solved first... which is not an easy issue to fix.

@TreemanKing
Copy link
Contributor Author

TreemanKing commented Sep 17, 2023

Treeman, did you consider the fix posted in the originial MC-868? Link to comment by FX - PR0CESS

Now I will mention why this should not be fixed xD

When changing this code you run into an issue with client rendering. Due to the client using interpolation, the client will render the minecart as if it's turning and then put it in the right place. So that issue should be solved first... which is not an easy issue to fix.

His solution checks a single co-ordinate/position.

@Owen1212055
Copy link
Member

Judging by MM's comment, it's probably not best for us to touch this since this may have implications in the whole technical world. This may also be behavior that is still used, so not sure if it's our place to patch something like this.

Additionally, I am not sure the code you are using will work on most setups due to it using awt classes.

Thank you for your contribution, regardless. :)

@Owen1212055 Owen1212055 closed this Jun 6, 2024
@TreemanKing TreemanKing deleted the FIX-MC-868 branch June 6, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants