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

Remove duplicate copies of core engine SDL files #1254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dgelessus
Copy link
Contributor

I noticed that a few internal engine SDL files (animation.sdl, avatar.sdl, etc.) exist in two separate copies in the repo: once under Scripts/SDL and once under Sources/Plasma/PubUtilLib/plSDL/SDL. It's been this way since the original open-source release. Thankfully, the two copies haven't diverged (except for formatting).

AFAICT, the files under plSDL are completely unused at the moment. In the plSDL/CMakeLists.txt, the SDL files are added as source files to the plSDL target, but that seems to do precisely nothing, because CMake has no idea what to do with .sdl files. At runtime the game uses the other copy from Scripts/SDL.

I asked in the OpenUru Discord and @Hoikas suggested:

The duplication is probably an artifact of how the open sourcing was initially handled. The Plasma repo has the engine copy that Cyan was probably developing with. The moul-scripts repo (Scripts/SDL) is just a copy of scripts assets required to run the game. We should probably delete the Scripts/SDL copy and install from the engine directory. Maybe a good issue to open.

So this PR does that.

The only downside to this approach is that you can no longer grab a complete set of SDL files straight from one directory in the repo, e. g. when setting up a server. (For example, I've set my local DIRTSAND test server to read the SDL files straight from Plasma/Scripts/SDL, so I can update them simply by pulling my clone of the Plasma repo.) The alternative would be to keep the core engine SDL files under Scripts/SDL and just delete the plSDL/SDL directory.

@Hoikas
Copy link
Member

Hoikas commented Oct 17, 2022

I like it! Thoughts, @Deledrius, @dpogue, @zrax?

@dpogue
Copy link
Member

dpogue commented Oct 17, 2022

In general, I like this.

I do wonder if we want to standardize on tabs vs spaces for SDL files. We seem to have a mix right now (the ones in PubUtilLib were using spaces, the ones in Scripts are a mix)

@dgelessus dgelessus mentioned this pull request Aug 8, 2023
66 tasks
@dgelessus dgelessus mentioned this pull request Aug 28, 2023
@Deledrius
Copy link
Member

What's the reasoning for these being needed specifically inside of the engine code?

@dgelessus
Copy link
Contributor Author

AFAIK, these are the only STATEDESCs that are used directly from the engine code - see plSDLTypes.h. All the other STATEDESCs are only used from Python and PRPs.

@dgelessus dgelessus force-pushed the remove_duplicate_sdls branch from f7a1846 to 10e11aa Compare September 17, 2023 13:13
@dgelessus
Copy link
Contributor Author

Rebased after #1477.

@Hoikas
Copy link
Member

Hoikas commented Sep 18, 2023

I think that, before we merge this, we need to make sure that DockerSand is equipped to deal with not all SDL files being in the Scripts/SDL directory. UruManifest (strictly speaking my responsibility) should be fine because it seems to simply ingest every SDL file that it can lay its hands on.

@dgelessus
Copy link
Contributor Author

Alternatively, we could just put all .sdl files in Scripts/SDL instead of trying to keep the core engine SDLs separate. It would make my life a bit easier too, because then I can continue pointing DIRTSAND directly at the Plasma/Scripts/SDL directory 😉

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.

4 participants