-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
Update the FlightLog (WIP) #5636
Update the FlightLog (WIP) #5636
Conversation
…er data foder Do not allow them to use popen
…s elements (or all of the ) into a flight log in their user data folder
…ab for the flight log with an options panel on the side to filter and save
…r what is shown based on checkboxes on the right Also split system and arrivals into separate events if there are other log entries between More consolidation and refactor of the code into flightlog.lua This changes the way that flightlogs are persisted but will still load the old version
Very cool! Combining all three tabs was my original intent, so nice to see this in place now. If I understand correctly, if
Without breaking backwards compatibility? I was going to complain about the "Save" text on the button, but note we're already sinners regarding this in the option screen. If you want to play around more: I also think the save/export functionality is slightly "immersion breaking", so I wonder if it could be less "front and center", like a button in the corner, opens a new window with export options. Not sure I understood the pruning part. Is there a function / method for pruning? When is this done? |
One overnight thought; I should do the pruning on system entries to consolidate them together if a filtered list is requested, although that leads to some questions about where to store any user annotations that are then edited/added. Pruning of the flightlog is done when a certain maximum number of entries are reached (was 1000 each for system entries and starport entries). Then when you add a new one, essentially the oldest one is deleted. With this change, once the total number of 3000 is reached the oldest is removed. I need to test that code though, adding it to my TODO list. I think I used the "Save" text simply because it was already localized :-) |
Re backward compatibility; old save files will load. Old versions of the game will not load new savefiles made with this version. AFAIK we have no way of storing upgrade/downgrade scripts such that we can load them into an older version of the game to load a newer savefile. I'm sure we can move that save button somewhere else :-) |
I'll defer UI decision to @nozmajner (TL;DR: Basically: should the export flight log to txt/html say "save" / "export" / icon?), and does the button break immersion. For localization: note, you need only update the en.json file, transifex does the rest. |
First rule of fightlog is we don't talk about fightlog.
Provisionally not a problem, though I may have suggestions once I fully review the code.
Also not a problem, savefiles are compressed. Just avoid writing large quantities of fully-translated strings when you could write the i18n key instead and you should be fine. As long as saves stay < 500kb there shouldn't be any issues.
I feel like this could be accomplished with a popover menu of some sort, but I don't think that's currently exposed to Lua at the moment. I can look into adding a binding for that given I implemented it during my Editor work.
No worries, we expect a rebase before final merge to clean up the history anyways.
This is expected behavior. We do not provide forwards compatibility; it is expected that savefiles created with a newer version of Pioneer are not loadable in an older version. On to the meat of the PR - I quite like what I'm seeing here! The layout has a minor case of hyperverticality, in that there's a huge amount of horizontal space left unused. It's also a little confusing to read, as your brain wants to read arrival -> departure but they're in inverse order unless you're reading the log bottom-to-top, which isn't clearly indicated. I'm not sure it's actually useful to show the player's current status as a persistent log entry, given that the information is always visible on the Personal Info screen anyways. You could move that info (along with the player's avatar) into the right hand column, but I'm not sure that really makes sense unless we consolidated Personal Info and Flight Log together (and provided a mechanism to tweak the player's face somewhere in the UI. I'm very happy with the automatic consolidation of arrival/departure entries in the log. If you have time and are willing, I'd suggest you might consider adding in a new log entry type that tracks the player's purchases while at a station - I've had a partially-baked branch to do that (flight-log-improvement which includes some other prototype stuff that's maybe not useful) but never was able to fully realize the design with the time I had available. I had originally conceived the design in the screenshot based on a spreadsheet Nozmajner was using to track game progress and profit; while it's non-trivial to fully track cargo items etc. to calculate profit, the basic design I think is solid and provides additional utility for the player. The layout of the table might be worth using to visualize all log entries in the flight log to look at the total lifetime profit of the player, for example. Regarding the technical implementation, the prototype-quality API I had involved passing around raw strings for the description of Flight Log entries; it might be better to provide an interface to register new flight log "row types" if you decide to adopt that code. That could then decouple the responsibility for formatting from the code that pushes entry data into the log. |
I swapped these when the log is displayed 'latest first', as I thought it's cleaner to show the departure first then, as it comes after the arrival, this way all the dates in the log are in the 'correct' chronological order. However, when it's displayed 'oldest first' they swap back. I'm in no way attached to this though :-) The personal info is really only useful when exporting, as I think then it is handy to see. So maybe I'll just always add it to an export and never display it. Would allow me to clean up the code too, so I think that's not too bad an idea. The commerce section is cool. I'll take a look at that :-) Maybe then I'll want a CSV export option!!! Yeah, the GUI is pretty sparse. I expect as a few more options for filtering get added that right hand bar will do more but having it pop-up/pop over would be good. Right now, unless the player adds long annotations or custom text there isn't much there. However, I'm now thinking that a table layout might be good too, with columns for the various elements. A lot of the types of log entry have a bunch of commonality and factoring that into the design might also help with code design too. Lots to think about. Thanks all. |
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.
Some nitpicks while you're thinking about the direction to take the PR:
- The FlightLog file is getting long enough to begin splitting into multiple files. It most likely makes sense with the amount of changes (in terms of preservation of git history) to move it to its own module folder (e.g.
modules/FlightLog/FlightLog.lua
) and split the FlightLogEntry file out from it. - Similarly, the export serializers for the flight log should not be implemented in a
pigui
file; that begins to stray into major separation-of-concerns / spaghetti-code territory. It's also better for flight log export to be a method in the Flight Log module proper so that code outside of the UI can access it. - We prefer not to use the Lua
goto
functionality, even though it's been used in a few places in the code. If you're able to make existing code cleaner and easier to follow by removing gotos "while you're there", I'd appreciate it. 😄
Nitpicks are good, it is how I'll learn! Just been thinking about this and I am leaning into the concept that a flat flightlog is the wrong one. Instead, the flightlog is a hierarchical, ordered, collection of events. System events contain starport events, which can contain trading events etc. Can then be displayed as a flat log if required but also helps with some of the conceptual problems, gluing together events etc... I'll think about that some more... |
Do move first then we'll start splitting it up to keep git history nice.
I had the same thought. Other than that, I have no more thoughts, than be ware of feature creep. Might be good to merge this asap, and then work on a v2 later (e.g. for commodity history). |
Thanks all. I've got a bit more tidy up to go, and I'm not sure I've got all the foundations laid quite right before merging. There is a little structural change I'd like to make that could change the save file format. I'll try and get it into shape soon. |
… the module Only export personal information when writing a log, don't show it on the gui ever. Remove the utils set class, it didn't add value
@JonBooth78 for your worries about savefiles, the upcoming February release is going to be a "savebump" release, which means we will intentionally be breaking backwards compatibility as part of the release. Don't feel the need to worry about maintaining compatibility with flightlog data at this point, especially with a series of PRs in-flight. We make no guarantees that the master branch is save compatible during a development cycle, so if you have to change the data model between now and a month from now there's no problem with that. I do appreciate the intention of maintaining savefile compatibility, just want to notify you that it's about to be moot anyways when #5622 is merged. |
@@ -170,6 +170,7 @@ local function startGame(gameParams) | |||
|
|||
-- XXX horrible hack here to avoid paying a spawn-in docking fee | |||
player:setprop("is_first_spawn", true) | |||
FlightLog.SkipFirstDocking() |
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 think this problem, as well as the one that is repaired by the previous line in a similar style, should not be repaired in this way. We just need to not generate a docking event when the game starts.
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.
If you're able to find a way to cleanly plumb that through the SpaceStation API, I'd love to merge something to that regard. This is definitely a horrible hack, as you can see from the code fragment 😄
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.
Yeah, I totally agree. I didn't want to get into too many tendrils of changes here but did want to fix the immediate issue with the flightlog.
Perhaps I should create a bug and then can resolve this separately and remove the hacks?
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 think yes, this problem should be solved separately. I would like to note that in #5619 the "state machine" that controls docking changes quite a lot, so I think it’s worth postponing the solution to this problem until the fate of the specified PR is decided.
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 will get to reviewing that PR sometime hopefully today.
… as an argument for the location of the file to open This ensures that files are opened within our sandbox and are read only if in the data folder. Addition of lua type annotations for the FileSystem functionality.
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.
Regarding the new FileSystem.lua - I'd recommend implementing the whole thing in C++ instead of having it partially in Lua. In general, I like how the system works, though I'd prefer the root filesystem to be specified via a user://
or data://
protocol-portion of the path rather than passing it as a separate parameter (this can be easily handled via starts_with_ci("user://", path)
in C++).
I'd be happy to put together an implementation of the API I'm envisioning if you're interested; I think this discussion is getting a bit beyond the initial purpose of the PR and the scope of "good first contribution".
All good. I actually started with an implementation like that, but then saw we had that type convention for the file/directory enumeration so thought it would be good to follow. For consistency, should that also change to use the same uri schema, then we can drop the constant altogether. I'm happy to keep going, but will spin this out into a separate pr I'm trying to balance a few things here...
Anyway, unless you have a burning desire and get to it first, I'll pull this out of here into a stand alone or and get onto it soon enough. |
OK for anyone watching, I just force pushed this same tree rebased on the successful merge of the Lua IO changes, which oddly hasn't shown up yet in github, and will start tidying up from here (before taking it further). |
This has now been replaced by #5666, as I made this PR no longer accept more changes from my branch. Sorry. |
User facing changes:
Potentially controversial changes I’m aware of:
Things I need to complete
Things I’ve tested:
Things I need to test:
Things I plan to add later: