fs2open.github.com
fs2open.github.com copied to clipboard
scp-fs2open
QtFRED campaign editor
First steps to implement #1693 Currently implemented as viewer-only of campaign files.
Lacks ability to add and properly modify branches, a campaign tree view, and saving of edits. It also says so in a warning dialog on launch. I'd like to get it merged in this stage to get some feedback, because I've done some things differently than they were done in FRED.
I'm making some changes to other parts of the code: Campaign names get stored with full paths, so I can open campaign files with the qt filechooser everywhere, qtfred loads subsystems mainhalls, cutscenes, and ranks to populate default campaign options, clang fixes in main.cpp and some changes to my checked data list model.
Finished for my part. There is no graphical representation of the campaign tree, nor a separate error checker (apart from saving), but everything else works.
I'm not put off by your comments, any shortness that may have created that impression is due to scarcity of time. I appreciate the help in improving this.
I didn't implement an error checker, so this fix does not apply. I'm using the in-game parsing functions as far as possible, so the bug did not appear. My UI is hopefully somewhat stricter than FRED in preventing invalid data.
The dialog should still have an error checker. There are actions the FREDder could take, such as setting the middle campaign branch to true, that are perfectly valid from a UI sense but still errors from a design sense. And others, like the internal_error lines in the original FRED dialog, are good defensive checks.
I'm doing this particular check when saving the campaign. I'll take another look at FRED's implementation of the error checker, but I do believe that error checking should not be different from saving a copy. True branch in the middle is indeed hard to check for during editing, but I consider it undesirable for the UI to accept this kind of semantic error, so I will prefer making the checks when editing.
@Goober5000 I've itemized the error checks in comments. Now tagged with the same numbers, I've expanded my "live" input validation to prevent the user from giving these kinds of bad input. It should be rather easy now to verify this simply by searching for a check number. Only in a few cases I'm running checks on saving, which result in warning/message dialogs. As I've added an option to save a copy, I believe this is sufficient. A "dry run" saving code path would also be possible, but, I believe, less useful than a copy for testing or manual proofreading.
Having resolved resolved previous change requests as far as I'm currently able to, I'd like to re-request reviews.
@the-maddin thanks for the updates!
Assuming that you've tested the functionality your PR adds, then I'm good with the PR whenever @z64555 is.
Note that, per the 22.2 RC cycle's feature freeze, even once the PR is approved, merging will need to wait until after 22.2 becomes Final.
Thanks again for taking the time to make this.
@the-maddin It looks like there are a few unaddressed comments from jg18 and z64555. Could you take another look at this?
AFAIK, jg18 is ok with the current state of the PR. As for @z64555 's last unresolved comment, which I seem to have overlooked, the enum variants are used only within the CampaignEditorModel, as CampaignBranchData::VARIANT. I'm adding some comments to clarify their use.
Since its been a few months I forget what exactly I was trying to hint at with the namespace question, maybe I was trying to push you towards using enum class since plain enum's are interpreted as a const int
This enum is nested two classes deep, and private to the outer class, so I never felt it was worth the effort to add another namespace for type safety. It only ever gets used in the most straightforward way of indicating an object's state.
Yeah uhm, The enum's don't actually get scoped regardless of what class they may be buried in. Each of the enum's defined values are effectively the same as const ints in the namespace that the enum was declared in.
You must either use a namespace bracket to scope them, or use enum class for the enum's values to ensure they will not collide with another enumeration, constant, or #define. Another old scheme to "scope" the enumeration is to add the name of the enum as a prefix to each value, such as "BRANCHTYPE_INVALID," but that gets old fast.
Possibly the least-effort fix here is to just put in enum class instead of just enum.
Declaring the enum inside a class has exactly the same effect as declaring it inside a namespace. Outside the inner class BranchData, the variants are used with that namespace, BranchData::VARIANT. Outside the outer class CampaignMissionData, they are not visible, as the namespace BranchData is private to that outer class.
When used inside BranchData, they shadow any global constant or enum variant, making a qualifier in that specific place unnecessary.
All right, either I'm remembering an ancient bug that's been addressed last century or misremembering how enum's scope.
If @jg18 is ok with the current state of the PR, as mentioned in a comment above, then we can merge this.