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

Pinning/Unpinning interaction and media promotion checks for bitECS + entity state API #6092

Merged
merged 29 commits into from
Jul 17, 2023

Conversation

stalgiag
Copy link
Contributor

@stalgiag stalgiag commented May 23, 2023

This PR ...

  • Adds utility functions for determining pinned/pinnable state and permissions
  • Removes Pinned and Pinnable components
  • Passes necessary keys to Reticulum for media promotion
  • Sends file_id to the backend when unpinning so that media can be demoted from permanent storage

To do this, I needed to add properties to the MediaLoaded component that store src and fileId values. This was necessary because the MediaLoader component is removed after load and those values are needed for media promotion. It is not ideal to duplicate the values in order to persist them. I mention possible alternatives in an inline comment.

Cases not handled by this PR ...
- Attempts to pin an object without being signed in silently reject. This is the existing behavior. This was not handled because (as far as I can tell) the bitECS code path doesn't have a solution for using performConditionalSignIn(). In the previous code path this function was passed as a construction parameter to a class instance of PinningHelper which was stored on APP. This does not seem appropriate for the new code path and likely exceeds the scope of this PR.

This PR is dependent on these changes to Reticulum so I will mark this PR as a draft until those changes are merged.

resolves #6086

@stalgiag stalgiag marked this pull request as draft May 23, 2023 15:59
src/bit-components.js Outdated Show resolved Hide resolved
@stalgiag
Copy link
Contributor Author

stalgiag commented Jun 2, 2023

This is ready for review again. It feels a lot cleaner with this approach. Thanks again for the suggestion. PR has a little more added to it in unexpected places now because I removed all use of Pinned and Pinnable components to prevent confusion about source of truth for this state.

I folded the unpin legacy components and deactivate files work into this PR. There is one point that I feel mildly uneasy about. I added an inline comment.

Copy link
Contributor

@johnshaughnessy johnshaughnessy left a comment

Choose a reason for hiding this comment

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

See the one comment about wanting to add a createEntityState call to where we loadLegacyRoomObjects (here and in the reticulum PR). Other than that, this looks good to me. Nice work!

src/utils/entity-state-utils.ts Show resolved Hide resolved
};

const unpinElement = (hubChannel: HubChannel, world: HubsWorld, eid: EntityID) => {
deleteEntityState(hubChannel, world, eid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of the way legacy room objects are handled, I think we should createEntityState in response to loading legacy room objects. Otherwise, when a client loads a legacy room object and synthesizes a "fake" create entity message for it, if the client moves the entity around, it will send "update" messages to reticulum, and reticulum will reject them (because it would have no stored entity state for it). This might be wasteful, so we may consider making an atomic "move RoomObject to EntityState" method later. But because I am concerned about an error causing us to lose information, I thought to do this non-destructively first. (And then delete both the RoomObject and the EntityState when an element is unpinned.)

@johnshaughnessy johnshaughnessy merged commit d52bd00 into master Jul 17, 2023
10 of 12 checks passed
@johnshaughnessy johnshaughnessy deleted the ecs-pin-permissions branch July 17, 2023 20:33
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.

Pinned objects can still be grabbed (newLoader)
3 participants