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

Add missing argument and improve documentation #830

Merged
merged 6 commits into from
Aug 9, 2024

Conversation

niekschoemaker
Copy link
Contributor

@niekschoemaker niekschoemaker commented Nov 9, 2022

I'm not 100% sure as to what the arguments do exactly because the behavior of the native changes depending on whether oal is enabled or not, not sure what's going on there, I'm assuming this is due to the missing argument.

Without OAL it always does the non flashing fade in, and with OAL it always does the flashing fade in, even though this is not the actual missing argument.

I'm not 100% sure as to what the arguments do exactly because the behavior of the native changes depending on whether oal is enabled or not, not sure what's going on there, I'm assuming this is due to the missing argument.

Without OAL it always does the non flashing fade in, and with OAL it always does the flashing fade in, even though this is not the actual missing argument.
@niekschoemaker
Copy link
Contributor Author

niekschoemaker commented Nov 9, 2022

I've validated that the argument is indeed missing from decompiled scripts, all calls did have 3 arguments and were along the lines of:
NETWORK::NETWORK_FADE_IN_ENTITY(iVar0, true, 1);

Copy link
Contributor

@4mmonium 4mmonium left a comment

Choose a reason for hiding this comment

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

Looks good, except for the added argument you declared, that's currently not supported. Requested some changes, thank you for your contribution 😊

@niekschoemaker
Copy link
Contributor Author

Looks good, except for the added argument you declared, that's currently not supported. Requested some changes, thank you for your contribution 😊

I've ammended the requested changes to the pull request, wasn't entirely sure which part could be done, but removed any "official" reference to the parameter and added a comment in the example and in the description about the missing parameter.

Copy link
Contributor

@4mmonium 4mmonium left a comment

Choose a reason for hiding this comment

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

Requested changes, but this time we know for sure what the parameters are 😊
Apologies for not researching this in full-detail earlier.

NETWORK/NetworkFadeInEntity.md Outdated Show resolved Hide resolved
@AvarianKnight AvarianKnight added the needs validation This looks good, but needs additional confirmation of suggested change. label Aug 7, 2024
NETWORK/NetworkFadeInEntity.md Outdated Show resolved Hide resolved
NETWORK/NetworkFadeInEntity.md Outdated Show resolved Hide resolved
NETWORK/NetworkFadeInEntity.md Outdated Show resolved Hide resolved
NETWORK/NetworkFadeInEntity.md Outdated Show resolved Hide resolved
@AvarianKnight AvarianKnight added ready-to-merge and removed needs validation This looks good, but needs additional confirmation of suggested change. labels Aug 9, 2024
@AvarianKnight AvarianKnight merged commit 0247530 into citizenfx:master Aug 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants