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

CREATE_PROJECT: Msvc dynamic modules #3405

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from
Draft

Conversation

@henke37
Copy link
Contributor

@henke37 henke37 commented Oct 5, 2021

This adds support for dynamic modules to CREATE_PROJECT, and specifically the msbuild target.

Right now there are a few things left to do:

  • Dynamically generate the def file
  • Solve the linking for 3d engines
  • Solve Common::Singleton being used outside of Common linking issues
  • Setup correct output locations for the plugin dlls
  • Check that other create_project outputs work
  • Implement the SCUMMVM_EXPORT define in non create_project builds
  • Check that everything works
@sev-
Copy link
Member

@sev- sev- commented Oct 6, 2021

MSVC is used only for development, and we use MinGW for the release builds. The changes in this PR are very invasive and affect a significant portion of our core. Hence my question: what is the benefit of this functionality?

@bluegr
Copy link
Member

@bluegr bluegr commented Oct 6, 2021

I agree with @sev-, the changes are too invasive. I'm all up for supporting dynamic modules in MSVC builds, but the changes needed should not affect existing code to such extend

@bluegr
Copy link
Member

@bluegr bluegr commented Oct 6, 2021

The changes to BASE are nice ideas and more straightforward, and can be submitted as a separate PR. Namely, 42e8bab and 82c94cb

@henke37
Copy link
Contributor Author

@henke37 henke37 commented Oct 6, 2021

The core issue here is telling the compiler & linker to use stuff from the main executable. All code references can be done in the .def file, but data requires a declspec in the code, doing it in the .def file is not enough.

As of the time writing, there are only 16 edited lines in non create_project c++ code related to this. I don't consider this too invasive. It's a testament of how few globals the core code has.

@sev-
Copy link
Member

@sev- sev- commented Oct 7, 2021

I hope I get an answer to my question regarding the benefit.

@bluegr
Copy link
Member

@bluegr bluegr commented Oct 7, 2021

My comment about the two aforementioned commits on BASE does not stand: changing the location of this code breaks compilation, because the relevant code needs to reside in the base directory.

The only benefit I personally see in this is debugging the dynamic module functionality through MSVC.

Having said that, there really shouldn't be any major changes to the engine code because of this. The changes here are too invasive and will need to be verified and debugged across the platforms we support. So, changes to the codebase outside of create_project should be minimal for this feature.

@henke37
Copy link
Contributor Author

@henke37 henke37 commented Oct 7, 2021

Bluegr pretty much nailed the benefit: It makes it possible to debug the plugin code for MSVC users. And when I get to it, other platforms that also use create_project. Those are included in my plan for this PR.

I agree that there should be no major changes to engine code here. Or base/common for that matter.

As for the code that breaks when moved, that I have not seen in my WIP builds. I will of course investigate this. If I break something, then I fix it.

However, I do not understand which changes qualify as "invasive". Are you talking about the need for the SCUMMVM_EXPORT define? While regrettable, they are few and tiny. A single "keyword" in less than twenty spots is not too many.

As for the changes to game engine folders, those are indeed worth scrutiny. They are essentially a revert of previous work on the plugin system. I am not an expert in make, so this is an area that I'm intending to examine in close detail.

As for testing, yes this will need careful testing. I'm not rushing this and I don't want to find out "later" that stuff broke.

henke37 added 23 commits Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants