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

Obfuscate player health to other players #127

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Beaness
Copy link

@Beaness Beaness commented Apr 2, 2023

This prevents clients knowing what another player their health is.

Obfuscation happens at entity tracker level & the health will always be obfuscated to 0.5 (unless you die).

Copy link

@iamnoksio iamnoksio left a comment

Choose a reason for hiding this comment

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

Should add a statement break under the found = true?

@Beaness
Copy link
Author

Beaness commented Apr 17, 2023

Should add a statement break under the found = true?

I'm unsure what happens if the HP is provided twice in the metadata, so it might be better to make sure that all metadata is checked.

@iamnoksio iamnoksio mentioned this pull request Apr 18, 2023
@cswhite2000
Copy link
Collaborator

This seems like something that should be behind a config option

@iamnoksio
Copy link

This seems like something that should be behind a config option

already done check the code --'

@Pablete1234
Copy link
Contributor

the code points to a config, but it doesn't create the config or declare how it's called in the file, so i'm not sure how that's going to even compile

@iamnoksio
Copy link

the code points to a config, but it doesn't create the config or declare how it's called in the file, so i'm not sure how that's going to even compile

do it instead of complaining is simple enough

@Pablete1234
Copy link
Contributor

You're requesting this code be merged upstream, and asking for comments and reviews from others.
Sure it's simple enough and i could do it, but it is not my responsability to fix other's PRs. You're responsible for the code you're requesting to merge, and it'd be nice if it was code that at least met a minimum of... compiling. Saying "do it yourself" to someone making a review or pointing something out is not productive. Finish your own homework on your own PR, thanks.

@iamnoksio
Copy link

You're requesting this code be merged upstream, and asking for comments and reviews from others. Sure it's simple enough and i could do it, but it is not my responsability to fix other's PRs. You're responsible for the code you're requesting to merge, and it'd be nice if it was code that at least met a minimum of... compiling. Saying "do it yourself" to someone making a review or pointing something out is not productive. Finish your own homework on your own PR, thanks.

never requested to be included, I've already included it in nPaper: sathonay/nPaper#17

@Pablete1234
Copy link
Contributor

never requested to be included

I assumed you were the original PR author just from a diff account, if that's not the case, then simply don't understand why you're interceding here to comments aimed at the PR original author. Good for you if you already fixed these changes and merged them into your own fork, but this is still @Beaness's PR requesting to merge this code here, and as i mentioned earlier, i'd be expecting the pr author to fix their changes before we can merge them.

Have a nice day.

@Beaness
Copy link
Author

Beaness commented Jul 5, 2023

Forgot to include the config file change in the commit. Will fix it once I got some time.

@Beaness
Copy link
Author

Beaness commented Aug 12, 2023

Config change is now inside the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants