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

Fix #322: onClientMarkerHit(Leave) ignores element's interior #1661

Closed
wants to merge 1 commit into from
Closed

Fix #322: onClientMarkerHit(Leave) ignores element's interior #1661

wants to merge 1 commit into from

Conversation

Unde-R
Copy link
Contributor

@Unde-R Unde-R commented Sep 10, 2020

According to #322 Looks like onClientMarkerHit doesn't care about element's interior and when I checked MTA's code looks like onClientMarkerLeave does the same thing, and for onMakerHit and onMakerLeave they do check for interior.

mtasa-blue/Server/mods/deathmatch/logic/CMarker.cpp

Line 315 in 3b95ff4

if (GetInterior() == Element.GetInterior())

@patrikjuvonen patrikjuvonen linked an issue Sep 14, 2020 that may be closed by this pull request
onClientMarkerHit ignores element's interior #322
Open
@patrikjuvonen patrikjuvonen added the bug Something isn't working label Sep 14, 2020
@patrikjuvonen
Copy link
Contributor

I believe an extra event parameter must be added for backwards compatibility reason, but this is debatable.

@Unde-R
Copy link
Contributor Author

Unde-R commented Sep 14, 2020

in discord, qaisjp said that we can fix it but without adding new parameter

@patrikjuvonen
Copy link
Contributor

in discord, qaisjp said that we can fix it but without adding new parameter

@qaisjp What's your idea on backwards compatibility or is this a screw-it issue?

@qaisjp
Copy link
Contributor

qaisjp commented Sep 14, 2020 •
edited

screw it situation (but maybe we should do this after 1.5.8 so we can watch the effects for longer?)

@qaisjp qaisjp self-assigned this Oct 17, 2020
qaisjp
qaisjp suggested changes Oct 17, 2020
View reviewed changes
Copy link
Contributor

@qaisjp qaisjp left a comment •
edited

Choose a reason for hiding this comment

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

This works fine and is technically safe for merging... but I think, reduce confusion, we should only merge this once we've fixed interiors appearing in all dimensions (#601 needs to be reimplemented in a separate PR).

@patrikjuvonen patrikjuvonen changed the title Fix onClientMarkerHit(Leave) ignores element's interior Fix #322: onClientMarkerHit(Leave) ignores element's interior Mar 19, 2021
@Lpsd
Copy link
Member

Lpsd commented May 4, 2021 •
edited

Why have we decided to randomly break backwards compatibility here when a reasonable, alternative solution already exists?

I think we should just add matchingInterior parameter to the client-side version of these events, as initially suggested.

@patrikjuvonen
Copy link
Contributor

Yeah, I think a parameter is good for now.

patrikjuvonen
patrikjuvonen suggested changes May 30, 2021
View reviewed changes
Client/mods/deathmatch/logic/CClientMarker.cpp
@@ -399,7 +399,7 @@ void CClientMarker::StreamOut()

void CClientMarker::Callback_OnCollision(CClientColShape& Shape, CClientEntity& Entity)
{
if (IS_PLAYER(&Entity))
if (IS_PLAYER(&Entity) && GetInterior() == Entity.GetInterior()) // Matching interior?
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, let's instead add the matching interior as an extra parameter to onClientMarkerHit call, just like matching dimension.

Client/mods/deathmatch/logic/CClientMarker.cpp
@@ -411,7 +411,7 @@ void CClientMarker::Callback_OnCollision(CClientColShape& Shape, CClientEntity&

void CClientMarker::Callback_OnLeave(CClientColShape& Shape, CClientEntity& Entity)
{
if (IS_PLAYER(&Entity))
if (IS_PLAYER(&Entity) && GetInterior() == Entity.GetInterior()) // Matching interior?
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, let's instead add the matching interior as an extra parameter to onClientMarkerHit call, just like matching dimension.

@patrikjuvonen patrikjuvonen marked this pull request as draft May 30, 2021 12:22
@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2022

This draft pull request is stale because it has been open for at least 90 days with no activity. Please continue on your draft pull request or it will be closed in 30 days automatically.

@github-actions github-actions bot added the stale Inactive for over 90 days, to be closed label Jan 7, 2022
@github-actions github-actions bot closed this Feb 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

This draft pull request was closed because it has been marked stale for 30 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qaisjp qaisjp qaisjp requested changes

@patrikjuvonen patrikjuvonen patrikjuvonen requested changes

Assignees
No one assigned
Labels
bug Something isn't working stale Inactive for over 90 days, to be closed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

onClientMarkerHit ignores element's interior
4 participants
@Unde-R @patrikjuvonen @qaisjp @Lpsd