Skip to content

Conversation

@Xymanek
Copy link
Member

@Xymanek Xymanek commented Jul 8, 2021

This reverts the DLC cooking approach (#46) and also implements some improvements to the previous/original approach:

@Xymanek Xymanek requested review from pledbrook and robojumper July 8, 2021 15:51
Copy link
Member

@robojumper robojumper left a comment

Choose a reason for hiding this comment

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

Seems fine to me. Not being able to rely on base game TFCs is unfortunate, but at least mods get their own GPCD, so there's also a win here. Seems there's no perfect solution 🤷.

Write-Host ""
Write-Host "Using cached release script packages"
Write-Host "If you've made changes to (or added) classes which are referenced by assets, please rebuild the mod in release (aka default) once to cache them"
Write-Host ""
Copy link
Member

Choose a reason for hiding this comment

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

Is this something a better incremental build system could do automatically? Worth filing an issue about?

Copy link
Member 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 how feasible it would be to get an automated logic that would achieve a perfect balance between detecting all relevant changes and avoiding rebuilding when the changes are irrelevant, but we could explore the details in a separate issue

Comment on lines +1056 to +1064
# Revert ini
try {
$this.sdkEngineIniContentOriginal | Set-Content $this.sdkEngineIniPath -NoNewline
Write-Host "Reverted $($this.sdkEngineIniPath)"
}
catch {
FailureMessage "Failed to revert $($this.sdkEngineIniPath)"
FailureMessage $_
}
Copy link
Member

Choose a reason for hiding this comment

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

This is the same behavior as before, but I think it's problematic that this prints an error message in the middle of the log (several screen heights above the success message) and then reports a successful build. I filed #54.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, this could be a nasty surprise for someone. Thanks for the issue

Copy link
Contributor

@pledbrook pledbrook left a comment

Choose a reason for hiding this comment

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

Things of note:

  • If you specify a map in sfMaps, there's no warning or error that I can see (it does error if an sfStandalone package is missing
  • If an entry is missing from ContentOptions.json, the file in ContentForCook is just ignore (I think this is OK, but wanted to mention it)

I haven't noticed anything else. Seems to work just fine.

@Xymanek
Copy link
Member Author

Xymanek commented Jul 9, 2021

If you specify a map in sfMaps, there's no warning or error that I can see (it does error if an sfStandalone package is missing

Both are "checked" when the cooked packages are copied to the staging directory. If the source packages don't exist the cooker won't produce a cooked one, so Copy-Item will fail

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.

"Assets cooking requested?" check is flawed Cease usage of SDK/Content/Mods directory for asset cooker Get rid of actually all base game packages

4 participants