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

System Editor #5625

Merged
merged 50 commits into from
Oct 17, 2023
Merged

System Editor #5625

merged 50 commits into from
Oct 17, 2023

Conversation

sturnclaw
Copy link
Member

They say a picture is worth a thousand words, but I've been working on this branch with every scrap of my free time for the last month+ so let's skip straight to the video.

2023-09-08.01-29-05.mp4

(Background music: "Eternity" by Stellardrone.)

This PR is currently in feature-complete state and waiting on #5622 and #5623 to be merged before being rebased and reviewed. I'm opening it now because it needs a lot of testing - this is intended to be the tool that all future modders and content creators use to define custom systems.

./build/editor --system is the editor invocation on linux systems; I leave it up to the CMake wizards to figure out how to translate that to Windows.

Testing Needed

Anyone even mildly able to compile from source is invited and encouraged to build this PR and try to use the tool to work on custom systems. This is a huge amount of new and changed code, and is replacing a large part of our current architecture and workflow for handling custom systems.

Use this tool to make new systems or improve randomly generated ones, and report back here with the results. If you run into a bug, please attach a zipped copy of the system definition you were working on and any necessary reproduction steps to this issue.

I specifically would like some poor brave soul to attempt to use it to convert all of our existing custom system definition files to the new JSON-based format the tool saves. This includes everything in data/systems/custom/ that isn't a "star record file" like data/systems/02_local_stars.lua.

At this stage, I believe the system editor will load a Lua custom system definition the exact same way the game loads it, so please verify that by loading the generated system JSON back into the tool and checking against a running copy of the game.

NOTE: If testing the editor, please check to ensure the UndoStack does not get corrupted (entry depth > 1). If you hold ALT, the undo stack window will give you a button to "reset" the current undo entry for easy testing.

Review Process

This is a huge PR, to put it bluntly. I don't expect any kind of substantive review (though it is welcome if anyone has the time). Ideally, a number of users test this PR and that functions as the review process, though if none come forward I'll most likely merge this PR in a few weeks and we can proceed with our (usual) process of "it's tested in master instead".

@impaktor
Copy link
Member

impaktor commented Sep 8, 2023

Fantastic work!

@impaktor
Copy link
Member

Hmm, this doesn't build for me. I tried removing the build dir, and re-do it. Not sure what the error is, as there's a lot of warnings.
Linker error?

/home/karlf/usr/src/pioneer/src/core/StringUtils.h:162:30: warning:   ‘SplitString* SplitString::iter::m_parent’ [-Wreorder]
  162 |                 SplitString *m_parent;
      |                              ^~~~~~~~
/home/karlf/usr/src/pioneer/src/core/StringUtils.h:131:17: warning:   when initialized here [-Wreorder]
  131 |                 iter() : m_str(), m_parent(nullptr) {};
      |                 ^~~~
In file included from /home/karlf/usr/src/pioneer/src/utils.h:8,
                 from /home/karlf/usr/src/pioneer/src/lua/core/Sandbox.cpp:8:
/home/karlf/usr/src/pioneer/src/core/StringUtils.h: In constructor ‘SplitString::iter::iter()’:
/home/karlf/usr/src/pioneer/src/core/StringUtils.h:163:34: warning: ‘SplitString::iter::m_str’ will be initialized after [-Wreorder]
  163 |                 std::string_view m_str;
      |                                  ^~~~~
/home/karlf/usr/src/pioneer/src/core/StringUtils.h:162:30: warning:   ‘SplitString* SplitString::iter::m_parent’ [-Wreorder]
  162 |                 SplitString *m_parent;
      |                              ^~~~~~~~
/home/karlf/usr/src/pioneer/src/core/StringUtils.h:131:17: warning:   when initialized here [-Wreorder]
  131 |                 iter() : m_str(), m_parent(nullptr) {};
      |                 ^~~~
[ 96%] Linking CXX static library libpioneer-lua.a
[ 96%] Built target pioneer-lua
make: *** [Makefile:136: all] Error 2
make: Leaving directory '/home/karlf/usr/src/pioneer/build'

@sturnclaw
Copy link
Member Author

Those warnings are harmless; the log fragment you've posted does not seem to include an error. Will be coming back to this soon (my break from system editor code was rather over-productive...) and will look at that error + finish blocking PRs.

@sturnclaw sturnclaw force-pushed the csys-editor branch 2 times, most recently from d3c5c03 to fbc4152 Compare September 14, 2023 22:06
@sturnclaw
Copy link
Member Author

I've (finally) implemented loading systems from JSON as part of the "normal" Galaxy startup flow, and also improved our .json.patch format handling to allow modifying array elements via special JSON Pointer syntax without completely overwriting the entire array. Shown here is a test "mod" file which makes some minor (and major) changes to the Sol system's JSON definition.

image

You probably shouldn't try to add or remove a system body via the JSON patch mechanism; it's not what it's intended for. However, this will allow mods to make minor edits to a custom system, without needing to redistribute (a copy of) the entire custom system definition.

@sturnclaw sturnclaw marked this pull request as ready for review October 8, 2023 17:33
@sturnclaw
Copy link
Member Author

Marking this PR as ready for review. I still have two minor things to implement (setting body heightmap file, and spaceport model name) and I consider this PR in a good state to merge.

I need to do a minor bit of cleanup on the git history (CMakeUserPresets.json snuck in somehow, it's supposed to be in .gitignore) but feel free to pick apart the code if interested!

@impaktor
Copy link
Member

Tried "new system", looks like there are elements missing / not shown:
2023-10-13-111513_5760x1080_scrot
Tried it again, and now I get list:
2023-10-13-111551_1621x922_scrot

Then I tried to "Open file", I see the open file window for 1 frame. I assumed it was some wonkyness due to my tiling window manager, so tried it again from a floating group (i.e. not in tiling mode anymore), but same.

I also, without any system loaded, you can still do edit -> sort bodies, which crashes the window.

The windows in the window menu seem to work (they're floating within the frame of the system editor window).

@sturnclaw
Copy link
Member Author

sturnclaw commented Oct 14, 2023

All of the issues mentioned in your comment (except the native file dialog issue) should be fixed. Unsure what's going on with the file dialogs, but that's nothing I have any ability to affect from within Pioneer's code, as the editor uses a third-party library wrapping platform-specific facilities to request an OS file picker.

@sturnclaw
Copy link
Member Author

I've added support for setting body heightmaps and explicitly specifying station models (for the total of 4 bodies across all our custom systems that use either of those features). I'd like to migrate those properties (and more) to a PropertyMap instead, but that's not within the scope of this PR and can be done separately without necessitating a savegame bump.

- Avoids needing to write additional setters unused by game code
- EditorAPI classes defined and used only in editor module
- ColorEdit3 specialization for Color4ub
- Reusable undo stack debug window
- EnumStrings-based EditEnum combo-box with undo support
Still in the early stages, can load and display custom system hierarchy and properties.

Can consistently round-trip most auto-generated system definitions, though requires more work for modified systems.
- Simple wrapper around toggling horizontal layout type
Most undo operations on plain data can be implemented as std::swap with extra trimmings.
Allow undo steps to implement their operations in terms of either separate Undo/Redo functions or a single Swap function.
- Split *EditorAPI classes into separate file
- Add new file for Undo helper classes
- Implement creation and deletion of non-root SystemBodies.
- Further work required to set sane/sensible defaults for new body and handle making a body the new root.
- Handles dropping a node before, after, or inside of a tree node
- Generic enough to support multiple datatypes
Allows using the same pre-created closure multiple times.
Icon rendering etc. is only an initial prototype, but it has achieved minimum viable functionality.
- UndoClosure wraps an UndoStep with a post-update closure
- Added UndoStep templates for managing insertion/removal of values from vectors
- Added UndoStep template for editing value inside of numerically-indexed container that may reallocate
- Edit sector path and position
- Add and edit system "comment" field
- Edit "other names" for system
- Allow the user to request load-time random generation of faction, lawlessness, and explored state
- Multi-functional class responsible for shortcut handling and menu rendering of predicated user actions.
- Wraps display and management of action shortcuts.
- Can be easily extended to use i18n and allow user-set keyboard shortcuts.
- Can be extended to render toolbar buttons instead of menu items.
- Reduces needed code to implement all contained functionality significantly.
- Load window layout from saved user settings if present
- Provide explicit functionality for user to reset window layout
- Provide access to various ImGui debugging tools
- Resets current undo frame in case of broken/un-closed undo entry
- Fire-and-forget modal management wrapping the peculiarities of working with ImGui modal popups
- Don't process action shortcuts while modal is open
- Implement "unsaved changes" nag modal
- Modals defined and managed separately from editor code
- Pick system modal serves as "new system" dialog as well as loading systems from galaxy
- Refactor viewport overlay rendering
- Allows using IsItemHovered() et. al. on body labels
- Populate Pi::luaNameGen to avoid crashes when generating random systems
- Undo state uses undo state hash for consistency
- Enable adaptive/swap-tear VSync so long updates don't immediately drop framerate to 20/30FPS
- Allow the user to toggle VSync on/off at runtime without restart
- Cydonia depends on a consistent seed value due to automatic starport repositioning
- Better interaction for creating new system / loading existing from Galaxy
- Set minimum vertical size for new system modal
- Fix crash when sorting bodies without system loaded
- Don't clear the current system unless replacing it with a new/loaded system
- Generate surface ports at random locations rather than in one great line
- Perturb orbital stations to make placement seem more random/built up over time rather than a line of stations sharing the exact same orbit
- Significantly reduce the worst-case number of starports generated for a body (on average, 1 starport per 0.6bil population)
@sturnclaw
Copy link
Member Author

Finally, I've adjusted spaceport generation to reduce the existing behavior of generating all spaceports in a nice orderly line around a polar orbit of the parent body (which included surface stations for legacy reasons).

I've also significantly reduced the worst-case number of stations that can be generated around a body; we had a truly ridiculous number of stations that were generated in some systems, well beyond what could be useful even with our goals for improved in-system gameplay opportunities. This also means that a tiny asteroid with a 200-500km radius will no longer be generated with six surface ports crammed together.

The end result is that generated starports around a high-population body go from looking like this:

To this:

@fluffyfreak
Copy link
Contributor

reduce the existing behavior of generating all spaceports in a nice orderly line around a polar orbit of the parent body

That doesn't seem like behaviour that I remember existing 🤔 when did that start?

@sturnclaw
Copy link
Member Author

when did that start?

Well before I started working on this PR. It wasn't very visible if you weren't looking for it specifically, but in problem systems with 4+ stations or surface ports it was very obvious. I think it was related to the hard-coded matrix math for setting the orbital plane; I've replaced most instances of that with a parameter-based helper which produces much more readable and intuitive results.

@sturnclaw
Copy link
Member Author

Per discussion on IRC, the above issue with missing file open dialog was due to impaktor's very stripped-down system missing all of zenity, kdialog, or matedialog to allow the program to open a native file dialog. At least one of those programs is available on all linux distros out of the box in my experience, unless the user has explicitly chosen to install nothing but their own user-provided list of packages.

Since that issue has been resolved as "user error" (:stuck_out_tongue:) I'm merging this PR.

@sturnclaw sturnclaw merged commit 5c6a6d5 into pioneerspacesim:master Oct 17, 2023
4 checks passed
@sturnclaw sturnclaw deleted the csys-editor branch October 17, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants