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

Remove agl folder #12

Merged
merged 4 commits into from
Mar 2, 2023
Merged

Remove agl folder #12

merged 4 commits into from
Mar 2, 2023

Conversation

ThePixelGamer
Copy link
Contributor

@ThePixelGamer ThePixelGamer commented Feb 25, 2023

This PR removes the agl folder, fixes include inconsistencies and adds a proper ordering for the includes.
Currently that looks like this:

  • regular <> includes
  • nn/nvn/vapours (folders from NintendoSDK)
  • files that start with sead
  • regular "" includes

This change is Reviewable

@@ -1,7 +1,7 @@
#pragma once

#include "agl/detail/aglMemoryPoolHeap.h"
#include "aglGPUMemBlock.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also get a fully qualified name, or stay like this? What about the ordering in such a case?

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 personally prefer the styling of not using folders when pulling files from the same folder but if leo wants I can change to include the common/ for consistency

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 64 of 64 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @leoetlino and @ThePixelGamer)


.clang-format line 40 at r1 (raw file):

    Priority:        3
  - Regex:           '^<'
    Priority:        1

Order it according to priority in here as well?

Code quote:

  - Regex:           '^<'
    Priority:        1

include/common/aglRenderTarget.h line 3 at r1 (raw file):

#pragma once

#include "aglTextureData.h"

Same here - including a path, maybe?


include/texture/aglTextureDataSerializer.h line 13 at r1 (raw file):

};

}  // namespace agl

Oh, look - a missing newline at the end of the file! If we're touching nearly every file here, fix that?

@ThePixelGamer
Copy link
Contributor Author

Order it according to priority in here as well?

  - Regex:           '^<'
    Priority:        1

nada in this case the sead files will be grouped with this regex if it's at the top (at least from my testing but I might've done something wrong?)

@ThePixelGamer
Copy link
Contributor Author

include/texture/aglTextureDataSerializer.h line 13 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Oh, look - a missing newline at the end of the file! If we're touching nearly every file here, fix that?

Done.

Copy link
Contributor Author

@ThePixelGamer ThePixelGamer left a comment

Choose a reason for hiding this comment

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

Dismissed @MonsterDruide1 from a discussion.
Reviewable status: 59 of 64 files reviewed, 3 unresolved discussions (waiting on @leoetlino and @MonsterDruide1)

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @leoetlino and @ThePixelGamer)


.clang-format line 40 at r1 (raw file):

Previously, MonsterDruide1 wrote…

Order it according to priority in here as well?

Oh, good point. Nevermind then.

@aboood40091
Copy link

Please use <> for includes not in the same folder. Also, the folders need renaming. Are you going to do that in a separate PR?

@ThePixelGamer
Copy link
Contributor Author

Which folders need renaming? If you could send it in the discord

@ThePixelGamer
Copy link
Contributor Author

Fixed and waiting for leo's input for the <> suggestion

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 32 of 32 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @leoetlino and @ThePixelGamer)


src/utility/aglParameterStringMgr.cpp line 9 at r3 (raw file):

ParameterStringMgr::ParameterStringMgr() {
#ifdef SEAD_DEBUG
    setNodeName("utility::ParameterStringMgr");

Was this an intended change?

@ThePixelGamer
Copy link
Contributor Author

Oops, was abusing vscode's replace feature

@ThePixelGamer
Copy link
Contributor Author

src/utility/aglParameterStringMgr.cpp line 9 at r3 (raw file):

Previously, MonsterDruide1 wrote…

Was this an intended change?

Done.

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @leoetlino, @MonsterDruide1, and @ThePixelGamer)

Copy link
Contributor

@MonsterDruide1 MonsterDruide1 left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @leoetlino)

@ThePixelGamer ThePixelGamer merged commit d88a29f into open-ead:master Mar 2, 2023
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.

3 participants