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

Jump to bottom

Simplify asset headers #2474

Merged
merged 2 commits into from Mar 2, 2023
Merged

Simplify asset headers #2474

merged 2 commits into from Mar 2, 2023

Conversation

xoascf
Copy link
Contributor

@xoascf xoascf commented Feb 14, 2023 •
edited by github-actions bot

  • Uses LF instead of CRLF
  • No spaces in empty lines
  • Simplified asset headers using an ALIGN_ASSET(x) macro

Build Artifacts

@briaguya-ai
Copy link
Contributor

@xoascf what happens when running ExtractAssets with these changes? My assumption is it leads to a giant git diff.

if we want to change these files we'll need to change ExtractAssets to generate them differently

@dcvz
Copy link
Contributor

dcvz commented Feb 15, 2023 •
edited

@xoascf this is where this is generated:
https://github.com/HarbourMasters/Shipwright/blob/develop/ZAPDTR/ZAPD/ZResource.cpp#L354
and
https://github.com/HarbourMasters/Shipwright/blob/develop/ZAPDTR/ZAPD/ZResource.cpp#L374

you should be able to make your changes there to generate the simplified headers

dcvz
dcvz reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

Overall looking pretty good. Just one question here

ZAPDTR/ZAPD/ZFile.cpp Outdated Show resolved Hide resolved
@xoascf xoascf force-pushed the simplify-asset-headers branch 3 times, most recently from 5430d81 to 29f048d Compare February 16, 2023 20:41
@xoascf xoascf requested a review from dcvz February 16, 2023 21:21
@dcvz
Copy link
Contributor

dcvz commented Mar 2, 2023

@xoascf could you please do one more rebase before I merge this? I'm not sure if we have the latest headers (at some point we added also the MQ headers).

@xoascf
Copy link
Contributor Author

xoascf commented Mar 2, 2023 •
edited

@xoascf could you please do one more rebase before I merge this? I'm not sure if we have the latest headers (at some point we added also the MQ headers).

Yes, shortly after that update, the headers were updated again for this PR, there should be no conflicts, but I will rebase again to make sure.

Update: There have been no changes after rerunning the ExtractAssets target for MQ.

dcvz
dcvz approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@dcvz dcvz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dcvz
Copy link
Contributor

dcvz commented Mar 2, 2023

Thanks for this @xoascf !

@dcvz dcvz merged commit d3f864c into HarbourMasters:develop Mar 2, 2023
7 checks passed
@xoascf xoascf deleted the simplify-asset-headers branch March 2, 2023 14:47
leggettc18 pushed a commit to leggettc18/Shipwright that referenced this pull request Mar 10, 2023
@xoascf @leggettc18
Simplify Asset Headers (HarbourMasters#2474)
8d45443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcvz dcvz dcvz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xoascf @briaguya-ai @dcvz