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

Clean up cutscene doc mess ups #749

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Lucas7yoshi
Copy link
Contributor

This fixes a incorrect link to another native and an uncompleted sentence

I did see e80e64b#r72290446 however i am not sure the meaning of this, or the resolution. @gottfriedleibniz

@Lucas7yoshi Lucas7yoshi force-pushed the fix-cutscene-docs branch 2 times, most recently from d965d62 to c3da8bc Compare April 28, 2022 14:49
@gottfriedleibniz
Copy link
Contributor

  • CUTSCENE/N_0x971d7b15bcdbef99.md was removed
  • CUTSCENE/GetCutsceneEndTime.md was given the alias 0x971D7B15BCDBEF99 and labelled with 0x011883f41211432a
  • CUTSCENE/N_0x011883f41211432a.md still exists.

Also, statements inferred from decompiled scripts, e.g., "Only referenced in script [X]", "Always false", "Usually [X]", or "i.e., Michael Lamar ..." should probably be removed or left in the comments section where the garbage from past imports live. These descriptions are often misleading and barely contribute to describing what those parameters actually are. Ideally, the "what it is" would be supplemented by "how it is used".

Unrelated comment: 0xE832D760399EB220 should probably have its carriage returns stripped.

@Lucas7yoshi Lucas7yoshi changed the title Clean up cutscene doc mess ups Draft: Clean up cutscene doc mess ups May 5, 2022
@stannum-cfx stannum-cfx added the Draft Work in progress PR. Remove label once PR is ready for review. label May 5, 2022
@Lucas7yoshi
Copy link
Contributor Author

@stannum-cfx This should be good now, added draft in the miniscule chance someone came by while I was trying to commit :p

@Lucas7yoshi Lucas7yoshi changed the title Draft: Clean up cutscene doc mess ups Clean up cutscene doc mess ups May 5, 2022
@stannum-cfx stannum-cfx removed the Draft Work in progress PR. Remove label once PR is ready for review. label May 6, 2022
Copy link
Contributor

@stannum-cfx stannum-cfx left a comment

Choose a reason for hiding this comment

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

Most things look good, only the hash change has me slightly concerned and is something I can't verify.

@@ -5,7 +5,7 @@ aliases: ['0x971D7B15BCDBEF99']
## _GET_CUTSCENE_END_TIME

```c
// 0x011883f41211432a
// 0x971D7B15BCDBEF99
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this will work properly, this might break stuff. @blattersturm could you verify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what my actions were prior when I first made the mistake so that change was based on Gottfried's response

Copy link
Collaborator

@AvarianKnight AvarianKnight Aug 9, 2024

Choose a reason for hiding this comment

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

Given that this is already "broken" with the way it currently because it will always return 0 due to sanitation, swapping this shouldn't break compatibility with anything.

This also didn't manage to overwrite the original native definition with an int return type so it shouldn't break C# codegen, we would just have to make sure that the native that was supposed to be in the enum slot gets named.

image

@stannum-cfx stannum-cfx requested a review from blattersturm May 6, 2022 12:43
@stannum-cfx stannum-cfx added the needs validation This looks good, but needs additional confirmation of suggested change. label May 6, 2022
@AvarianKnight AvarianKnight added merging-blocked PR is waiting for something else to be merged. and removed needs validation This looks good, but needs additional confirmation of suggested change. labels Aug 9, 2024
@AvarianKnight AvarianKnight added ready-to-merge and removed merging-blocked PR is waiting for something else to be merged. labels Sep 19, 2024
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.

5 participants