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

764 make helpsystem work again and provide additional content #781

Merged

Conversation

SarahAugustinowski
Copy link
Collaborator

@SarahAugustinowski SarahAugustinowski commented Sep 23, 2024

This should close #764 and make the HelpSystem work again. For that an empty "HelpSystem" with the follwing scripts was added to the world again:
grafik
Changes in the HelpSystemEntry Prefab increase the size of the decor frame around the video player.

Some video were updated others are new. While trying to update videos some issue (#776 and #778) occurred.
At the moment user gets an notification about the HelpSystem and how to enter it, that might be changed to something like #777.

After approving this PR (especially how new entries are now created with a json file) I can write a wiki entry on how to maintain this.

It might be a good workflow to ask developers for new HelpEntries before merging PRs with new features to the master.

@falko17
Copy link
Collaborator

falko17 commented Sep 23, 2024

Note that the pipeline is currently failing because the help system videos aren't in LFS, even though they should be (due to their size). To fix this, we need to do an interactive rebase1, as they will otherwise be permanently in the history of the repository and will unnecessarily inflate it and slow it down. I recommend that we do this before the reviews, because otherwise the reviews will reference non-existing commits and would, for example, not be automatically applicable.

After approving this PR (especially how new entries are now created with a json file) and I can write a wiki entry on how to maintain this.
It might be a good workflow to ask developers for new HelpEntries before merging PRs with new features to the master.

I agree, both of these sound like good ideas.
(Actually, for the latter, maybe it'd be nice to create a PR template with a checkboxed list, as many other open-source projects do, then we could also add some other common pitfalls in there, such as formatting, the git hook checks, etc. We can do this in a separate pull request.)

Footnotes

  1. Probably on commit 7032cad, which we want to edit to move those files into LFS.

@SarahAugustinowski
Copy link
Collaborator Author

SarahAugustinowski commented Sep 24, 2024

Note that the pipeline is currently failing because the help system videos aren't in LFS, even though they should be (due to their size). To fix this, we need to do an interactive rebase1, as they will otherwise be permanently in the history of the repository and will unnecessarily inflate it and slow it down. I recommend that we do this before the reviews, because otherwise the reviews will reference non-existing commits and would, for example, not be automatically applicable.

@falko17 Can I specify the lfs-repo in the edit so the commit goes there instead? Or just delete the files? I would be happy to get some help with that :)

@falko17
Copy link
Collaborator

falko17 commented Sep 24, 2024

@SarahAugustinowski I took some time to fill in the relevant sections in our Git FAQ just now, which I totally forgot existed 😅

In your case, the relevant section is "How do I rebase a branch?" In the rebase, you will need to do an edit on commit 7032cad. During the edit, you can add the moved files to LFS—see section "How do I add a large directory to LFS", in this case for directory Assets/StreamingAssets/HelpSystem/Videos.

If there are any issues or if anything is unclear, please message me on Mattermost, and I can help you on there.

@SarahAugustinowski SarahAugustinowski force-pushed the 764-make-helpsystem-work-again-and-provide-additional-content branch from db0b11a to 24e5e3a Compare September 24, 2024 23:39
@falko17 falko17 force-pushed the 764-make-helpsystem-work-again-and-provide-additional-content branch from 24e5e3a to 7ee9f53 Compare September 25, 2024 16:52
@falko17 falko17 force-pushed the 764-make-helpsystem-work-again-and-provide-additional-content branch from 7ee9f53 to 4ec32cf Compare September 25, 2024 16:54
@falko17
Copy link
Collaborator

falko17 commented Sep 25, 2024

Issues regarding LFS are resolved now, so the PR should be ready for review. (The pipeline is still failing, but this is due to game-ci/docker#250 and happens on every PR right now.)

@SarahAugustinowski
Copy link
Collaborator Author

Issues regarding LFS are resolved now, so the PR should be ready for review. (The pipeline is still failing, but this is due to game-ci/docker#250 and happens on every PR right now.)

thank you for your help! :) @falko17

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

@SarahAugustinowski It looks like the video Assets\StreamingAssets\HelpSystem\Videos\Navigation.mp4 is showing the runtime configuration menu, but not the navigation.

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

"Hide node" is work in progress. No video and no text for this action.

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

In the video on "Show code" an error message appears because the issues cannot be dowloaded from the dashboard.

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

"Search for a node" is still work in progress.

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

The help text is not automatically split into the existing line space. This way long help text will be truncated. The lower scroll bar has no effect.

@SarahAugustinowski
Copy link
Collaborator Author

@SarahAugustinowski It looks like the video Assets\StreamingAssets\HelpSystem\Videos\Navigation.mp4 is showing the runtime configuration menu, but not the navigation.

@koschke There should not be any video at this path, it should be placed here: "\SEE\Assets\StreamingAssets\HelpSystem\Videos\Navigation\Navigation.mp4"

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

@SarahAugustinowski It looks like the video Assets\StreamingAssets\HelpSystem\Videos\Navigation.mp4 is showing the runtime configuration menu, but not the navigation.

@koschke There should not be any video at this path, it should be placed here: "\SEE\Assets\StreamingAssets\HelpSystem\Videos\Navigation\Navigation.mp4"

Sorry, I missed the directory. The path you gave was intended. Still, this video has the wrong content.

@koschke
Copy link
Collaborator

koschke commented Sep 26, 2024

We will need to re-organize the structure of the help system content. Yet, that is beyond @SarahAugustinowski's task. We need to do that later.

@SarahAugustinowski
Copy link
Collaborator Author

@SarahAugustinowski It looks like the video Assets\StreamingAssets\HelpSystem\Videos\Navigation.mp4 is showing the runtime configuration menu, but not the navigation.

@koschke There should not be any video at this path, it should be placed here: "\SEE\Assets\StreamingAssets\HelpSystem\Videos\Navigation\Navigation.mp4"

Sorry, I missed the directory. The path you gave was intended. Still, this video has the wrong content.

Still weird :-D I have not pulled from the branch after the problems with lfs, but locally it is fine:
grafik

@SarahAugustinowski
Copy link
Collaborator Author

SarahAugustinowski commented Sep 26, 2024

"Hide node" is work in progress. No video and no text for this action.

Yep, the Entry should be updated after #776 is fixed.

@SarahAugustinowski
Copy link
Collaborator Author

The help text is not automatically split into the existing line space. This way long help text will be truncated. The lower scroll bar has no effect.

The tick for horizontal scrolling was missing, the lower scrollbar works now.

Copy link
Collaborator

@koschke koschke left a comment

Choose a reason for hiding this comment

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

I made a few minor changes myself. Looks good now and can be merged.

@koschke koschke merged commit 5f655c3 into master Sep 27, 2024
3 checks passed
@koschke koschke deleted the 764-make-helpsystem-work-again-and-provide-additional-content branch September 27, 2024 08:44
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.

Make HelpSystem work again and provide additional content
3 participants