Skip to content

feat: Save slots#692

Open
alex-y-z wants to merge 5 commits into
mainfrom
users/alex-y-z/save-slots
Open

feat: Save slots#692
alex-y-z wants to merge 5 commits into
mainfrom
users/alex-y-z/save-slots

Conversation

@alex-y-z
Copy link
Copy Markdown
Collaborator

PlayerDataStoreService wrapper for save slots.

Provides an interface for creating, manipulating, and observing slots such that consumers can seamlessly react to slot switching.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 12, 2026

Test Results

ℹ️ No changed packages with test targets were discovered for this PR. · View logs

Deploy Results

ℹ️ No changed packages with deploy targets were discovered for this PR. · View logs

Comment thread src/saveslot/src/Server/Binders/HasSaveSlots.lua Outdated
Comment thread src/saveslot/src/Server/SaveSlotService.lua Outdated
Comment thread src/saveslot/src/Shared/HasSaveSlotsInterface.lua
Comment thread src/saveslot/src/Shared/HasSaveSlotsInterface.lua
Copy link
Copy Markdown
Owner

@Quenty Quenty left a comment

Choose a reason for hiding this comment

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

Looks good overall. Left a ton of comments.

]=]
function HasSaveSlotsClient.PromiseSetSlotMetadata(
self: HasSaveSlotsClient,
slotId: string,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Typically I'd type slotId to make future refactors safer but it's ok as-is too.

Gets the last active slot ID
]=]
function HasSaveSlotsClient.PromiseLastActiveSlotId(self: HasSaveSlotsClient): Promise.Promise<string?>
return self._remoting.PromiseLastActiveSlotId:PromiseInvokeServer()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Might be worth some fancier cache management, potentially internally a Boolean attribute for loaded and an attribute for the slot id, so that we're effectively cached on the client instead of requesting server a lot.

This will happen on initial game load, potentially many systems may promise the slot id.

Refreshes the active slot summary
]=]
function HasSaveSlotsClient.PromiseRefreshActiveSlotSummary(self: HasSaveSlotsClient): Promise.Promise<any>
return self._remoting.PromiseRefreshActiveSlotSummary:PromiseInvokeServer()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Preferably we don't have to leak this state externally but it's ok as-is and I don't see a clean way right now to avoid this.

type SaveSlotStruct = {
folder: Folder,
attributes: any,
maid: Maid.Maid,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is a tad risky given it's not a fully managed object, we could forget to cleanup this maid structurally.

Safest option? Store the maid in a parallel structure and make this pure data.

local data = {
SlotId = slotId,
SlotIndex = slotIndex,
SlotName = (metadata and metadata.SlotName) or `Slot {slotIndex}`,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should maybe rename to UnfilteredSlotName so we remember to filter the text.

Maybe not a big deal though.

slotId: string,
data: SaveSlotData.SaveSlotMetadata
): Promise.Promise<any>
if data.SlotId and (data.SlotId ~= slotId) then
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just use SaveSlotData:IsStrictData() call here to fully check and get a good error message.

end)
end

function HasSaveSlots._refreshActiveSlotSummary(self: HasSaveSlots): ()
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

It would definitely be nicer if this was just managed using Rx for the user. More guaranteed safety.

end)

self._cmdrService:RegisterCommand({
Name = "active-save-slot",
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Maybe should prefix with get? Also fine as-is. I wish our command scheme was better named.

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.

2 participants