- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 67
feat: Player Data Storage #205
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
base: master
Are you sure you want to change the base?
Conversation
| Hey thanks for getting started on this! My general plan for storing entities as a whole is to use sqlite (or turso depending on it's progress) to store them all in a normal sql database, as opposed to the kv format of lmdb. If you'd be comfortable starting on that that'd be great, otherwise just use lmdb for now and I can move it over later. | 
| Ok, I'll switch to SQLite and use the idea of the playerdata folder but in a SQL database (playerdata.db) 👍 | 
c6ba358    to
    d9b0bfa      
    Compare
  
    … into feature/player-state
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.
Mostly good changes, but a lot of OOP patterns that don't really fit the project, especially the PlayerData stuff since that can actively harm bevy's parallel processing capabilities.
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.
Implementing this as a trait doesn't make much sense since it has quite a different use-case as the KV store, especially since we'd like be storing entities in here as well.
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 separated them because both are banks and have, in my view, common functions and, if they decided in the future to change the bank that stores the players, I think it would make things easier...
444bc64    to
    b70b7b2      
    Compare
  
    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've reviewed what I could. Not that experienced with database, @ReCore-sys would be able to review that section properly. But is SQL Injection a thing we should be worried about? I reckon not, since we have control over the database. And also traits for database seems interesting, again, not an expert so recore will have to check that out.
| use bevy_ecs::{event::EventReader, prelude::Res, system::Commands}; | ||
| use ferrumc_core::conn::player_disconnect_event::PlayerDisconnectEvent; | ||
| use ferrumc_state::GlobalStateResource; | ||
|  | ||
| pub fn handle( | ||
| mut cmd: Commands, | ||
| mut events: EventReader<PlayerDisconnectEvent>, | ||
| state: Res<GlobalStateResource>, | ||
| ) { | ||
| for event in events.read() { | ||
| let uuid = event.identity.uuid.as_u128(); | ||
| let username = &event.identity.username; | ||
| if let Err(e) = state.0.world.save_player_state(uuid, &event.data) { | ||
| tracing::error!("Failed to save player state for {}: {}", username, e); | ||
| } else { | ||
| tracing::info!("Player state saved for {}", username); | ||
| } | ||
| cmd.entity(event.entity).despawn(); | ||
| } | ||
| } | 
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.
from what i see, this is an event handler, why is this in the packet handler dir? is confusing if its the disconnect event received from connection killer and server kill.
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.
Honestly, I didn't know where to put it, after all, we didn't have a folder called events and I didn't know whether to put it inside systems or packets (the reason for putting packets is because of the "head_rot" event)
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.
head rot event is most likely being called from a packet. on the other hand the event you made has nothing to do with packets. why not make a separate module specifically for events? i reckon it'd be best?
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 agree, having it here makes little sense
| use bevy_ecs::{event::EventReader, prelude::Res, system::Commands}; | ||
| use ferrumc_core::conn::player_disconnect_event::PlayerDisconnectEvent; | ||
| use ferrumc_state::GlobalStateResource; | ||
|  | ||
| pub fn handle( | ||
| mut cmd: Commands, | ||
| mut events: EventReader<PlayerDisconnectEvent>, | ||
| state: Res<GlobalStateResource>, | ||
| ) { | ||
| for event in events.read() { | ||
| let uuid = event.identity.uuid.as_u128(); | ||
| let username = &event.identity.username; | ||
| if let Err(e) = state.0.world.save_player_state(uuid, &event.data) { | ||
| tracing::error!("Failed to save player state for {}: {}", username, e); | ||
| } else { | ||
| tracing::info!("Player state saved for {}", username); | ||
| } | ||
| cmd.entity(event.entity).despawn(); | ||
| } | ||
| } | 
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 agree, having it here makes little sense
| for (entity, conn, identity, position, on_ground, rotation) in query.iter() { | ||
| if state.0.players.is_connected(entity) { | ||
| let player_disconnect = PlayerDisconnectEvent { | ||
| data: PlayerData::new(position, on_ground.0, "overworld", rotation), | 
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.
Why are we including this data here? The event handler can just fetch this data if you give it the entity
| @@ -0,0 +1,33 @@ | |||
| use crate::errors::StorageError; | |||
|  | |||
| pub trait Database { | |||
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.
This trait is largely not going to work when we need to start doing more complicated SQL queries and we won't be doing any dynamic dispatch shenanigans with multiple DB backends, so I'm not sure if using a trait here is the right move
|  | ||
| fn insert(&self, table: &str, key: Self::Key, value: Self::Value) -> Result<(), StorageError> { | ||
| let conn = self.open_conn()?; | ||
| let json_val: Value = | 
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.
Why are we using json here? we know the fields, you can just store those as columns in the table, that kinda the whole point of SQL databases
|  | ||
| impl World { | ||
| pub fn save_player_state(&self, key: u128, state: &PlayerData) -> Result<(), PlayerDataError> { | ||
| self.player_state_backend.create_table(TABLE_NAME)?; | 
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.
You don't need to create a table each time you call, just generate the tables in the setup/init stage and assume it's there, sqlite will error out if it's not
Currently, my proposal is to leverage the same database used for world storage (LMDB) and persist player data there. This follows the general idea from Bukkit/Spigot/Paper, where player data is stored inside the world folder.
But I'm still considering whether storing all player data inside data.lmdb is the best approach, or if a dedicated playerdata folder inside the world directory—where each player's data would be stored separately by UUID—might provide better organization and flexibility.
For now, the implementation will move forward with LMDB, but I’m open to feedback on which design would be preferable long-term.
Todo (work in progress, will update as needed):