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

enum reorganization or merging project into Outpost2DLL #22

Open
Brett208 opened this issue Dec 3, 2019 · 3 comments
Open

enum reorganization or merging project into Outpost2DLL #22

Brett208 opened this issue Dec 3, 2019 · 3 comments

Comments

@Brett208
Copy link
Member

@Brett208 Brett208 commented Dec 3, 2019

As discussed in PR #20, the organization of NonExportedEnums was currently considered sub-optimal. Proposed solutions were:

  1. Move NonExportedEnums.h from Outpost2DLL into OP2Helper. Might also be worth changing the filename and or splitting enums into separate friles from NonExportedEnums.

  2. Move OP2Helper into Outpost2DLL. We didn't really discuss what this looks like. Whether it was moving a OP2Helper as a separate project file or including it as bundled in the same project file as Outpost2DLL.

If OP2Helper is included in Outpost2DLL's project file it would:

  1. Encourage OP2Helper code to be present in non-mission applications that depend on Outpost2DLL. Maybe not a big deal?
  2. Reduce project file count and post build script size
  3. Reduce number of includes required in a level project

If OP2Helper is not included in Outpost2DLL, I suppose the opposite would just be true.

I think I would be okay supporting either solution.

@DanRStevens

This comment has been minimized.

Copy link
Member

@DanRStevens DanRStevens commented Dec 8, 2019

I believe the linker does a pretty good job of stripping out unused code. If OP2Helper code becomes part of the Outpost2DLL project, but isn't used, it likely won't add to the output executable size.

It probably would be a good idea to spilt the enum file into multiple files. This is true regardless of which project they end up being in.

If merging projects, and namespace pollution is a big deal, we could still have different top level header files for the 3 sections. There would be "Outpost2DLL.h" (game/), "Outpost2App.h" (shell/), and "OP2Helper.h" (we haven't yet discussed a folder name for this code if it is merged).

Related to the above, as there is some discrepancy between the header name and the folder they represent, we might consider updating names at some point. For renamed headers, we could offer compatibility shim header files to keep old code compiling, with perhaps a deprecation warning in them.

As OP2Helper code must necessarily include Outpost2DLL code, we could explicitly specify that including OP2Helper also includes all the Outpost2DLL names (that's already how things work). That would be a path for reducing header includes, while getting access to both projects.

Merging would make Outpost2DLL a proper static library, rather than a quasi static library which just contains the already pre-compiled Outpost2DLL.lib file. It would however end up having two static libraries. One being compiled and provided, and one being imported. Related, we probably should have named Outpsot2DLL.lib as simply Outpost2.lib. This is an import library for the project, not the compiled export library, so it's name probably shouldn't exactly match the project name. It is effectively the output of the Outpost2.exe project, which we don't have source code for. The header files from Outpost2DLL are effectively the import headers that we expect the Outpost2.exe project to have provided. You might think of the Outpost2DLL headers and static library as a binary release of the Outpost2.exe project. Sorry if that all gets a bit muddy and hard to follow.

@Brett208

This comment has been minimized.

Copy link
Member Author

@Brett208 Brett208 commented Dec 9, 2019

Based on the last comment, I think @DanRStevens is in favor of moving OP2Helper into Outpost2DLL. I generally agree with the made comments.

To condense it:

  1. Move OP2Helper project into Outpost2DLL as a subfolder in the root Outpost2DLL repository.
  2. Add OP2Helper project to the Outpost2DLL solution.
  3. Move non-exported enums into the OP2Helper project.
  4. Split non-exported enum into separate headers for each enum. Use naming convention Enum prefix and then Enum name for filename.
  5. Enum include shim??? I'm not sure about this one. Ideally, people should have been including Outpost2DLL.h to use the non-exported enums. If we add the deprecation within Outpost2DLL.h, won't it just tell everyone they should be using OP2Helper instead of Outpost2DLL.h?
  6. Rename file Outpost2DLL.lib to Outpost2.lib and update project file with new name.

Also, I am assuming it is possible, but I don't know how to merge two repository histories into one. IE keep OP2Helper's history when it is pushed inside of Outpost2DLL.

@DanRStevens

This comment has been minimized.

Copy link
Member

@DanRStevens DanRStevens commented Dec 9, 2019

This history will be an interesting problem. It should be possible, since I know the Git model supports independent branches. GitHub uses that for the GitHub Pages feature. I'm not sure of the specifics though.

I believe BlackBox or Arklon were once talking about merging projects. I don't remember for certain though, or know quite what they had in mind. For a change this big, we should probably consult other people.

The shim comment was in case we want to rename Outpost2DLL.h, Outpost2App.h, or OP2Helper.h. That is, we can rename the top level header includes with a compatibility shim that just issues a deprecation warning, and then includes the new top-level header name. Old code would still compile, but with a warning.

We would need to settle on a folder structure. It would be nice if we could have folder names that matched up with top-level header names. I don't have anything satisfying at this time. I was thinking of "opu/" for our custom code such as OP2Helper, but that's way too vague and generic, particularly considering we have other projects that may or may not be merged at a later date.

We might need to sit on this idea for a little while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.