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
setPedStat for client side peds #106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good all in all.
In addition to the in-code comments, you could avoid the double m_pScriptDebugging->LogCustom(...)
by something like this:
// ...
if (!argStream.HasErrors())
{
pPed->SetStat(usStat, fValue);
// ...
}
}
// at the end:
if (argStream.HasErrors())
m_pScriptDebugging->LogCustom(luaVM, argStream.GetFullErrorMessage());
if ( !argStream.HasErrors () ) | ||
{ | ||
// Valid ped? | ||
if ( pPed ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is not necessary as the error case is already handled by CScriptArgReader::HasErrors
m_pScriptDebugging->LogCustom ( luaVM, argStream.GetFullErrorMessage ( ) ); | ||
} | ||
else | ||
m_pScriptDebugging->LogBadPointer ( luaVM, "ped", 1 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed then.
Implemented Jusonex's feedback, fixed not checking if stat was < 0 and changed message about max stat from 231 (highest on MTA wiki) to 343 which is what NUM_PLAYER_STATS is.
Thanks for the review, I've made a new commit. About that if ( pPed ) I assumed that it was needed as many functions in that file have those checks, like getPedStat does. If these checks are useless then can I remove them all in a new patch? |
I guess something like this needs adding: |
I seem to have messed up by using the master branch for a pull request so going to close this and make a new one that uses a branch. |
No description provided.