-
Notifications
You must be signed in to change notification settings - Fork 8
Optional features for auto-record, save in map seed subdirectories and choosable file formats #70
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
Conversation
veger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thank you for your contribution!
It looks nice when looking at the code changes, but I want to try it out in the game as well to double check.
Ideally, the features would have been in separate PRs, so they could be merged individually. But if you are willing to add (a) changelog entry(ies), I can squash merge this one also. Assuming my playtesting and/or more careful reviewing will not show issues. But this has to be done tomorrow, as it is pretty late already.
|
Great! I'm happy you like it so far! I added the changelog entry for my changes. |
| savePath = string.format( | ||
| "%s/%s/%d/%010d-%s.%s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should store (cache) savePath in playerSettings...? 🤔
There are so many components to build the path and only the screenshotNumber is changing each save. All the others are static and only change when the player settings are changed.
Maybe it is better to cache this outside of playerSettings, as we then initialize on startup and do not require a migration to existing saves...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll create a ticket for this, as it is a separate issue and should not be part of this PR.
Camera name is also part of it, so caching this properly requires more changes than I initially imagined...
It is probably not worth it (especially with the risk of forgetting of updating the cached path in some scenario).
|
Thanks again for your contribution! I'll make a release this evening (maybe something pops up in my mind that needs changing 😉 ) |
I made some changes with features I wanted. Everything is optional and has to be enabled in the mod-settings.
The default behaviour of the mod is unchanged (except for the bugfix ofc).
My intention with these changes was to make a more hand-off style of use possible.
I played a few hours without issue and tryed to test the changes to the best of my abilities.