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

Added Glue #5

Open
wants to merge 12 commits into
base: sdl2
Choose a base branch
from
Open

Added Glue #5

wants to merge 12 commits into from

Conversation

weigert
Copy link

@weigert weigert commented Apr 10, 2023

This PR adds the necessary glue to make the code compilable. External symbols and functions are defined in pure stub form.

A Makefile is included which successfully compiles the main function into an executable using make all. Finally, .gitignore is added to hide the build+run artifacts.

The code has been tested on Mac w. an M1 chip (arm), but I will test it later today with an x86 amd GNU/Linux machine.

The executable builds and runs successfully. Only minor modifications are required to the g_src code, mainly include directives.

Merging this PR would help guarantee that any changes to g_src are valid code changes, helping development for anybody interested.

@myk002
Copy link

myk002 commented Apr 10, 2023

very nice. the next step (if Putnam is amenable) could be hooking up GitHub build actions to automatically ensure that PRs build successfully before merging. If you're interested, you can base a configuration on DFHack's here: https://github.com/DFHack/dfhack/blob/develop/.github/workflows/build.yml

@weigert
Copy link
Author

weigert commented Apr 10, 2023

That would be convenient. It depends on how much this repository is intended to receive community contributions. I think it can always benefit from "more hands/eyes on deck", but then PR management becomes a new set of tasks for @Putnam3145... so it is really up to them.

The current form of the glue is "bare bones". If some details could be added about (roughly) what the external functions do, one could more accurately mock for a debug environment.

Still, being able to test if code "is compilable" means that any potential code recommendations will at least be valid.

Also, if cmake is preferred over make, or any other adjustments should be made to the "glue build", I am happy to fix the PR with any requirements.

…extra linkage and g++11 which has library support for semaphore and syncstream. removed self-credit in Makefile
@weigert
Copy link
Author

weigert commented Apr 10, 2023

Can confirm the branch builds and runs on amd/x86 w. ubuntu.

@@ -33,8 +33,8 @@ using std::queue;
#include <SDL2/SDL.h>
#include <SDL2/SDL_thread.h>
#ifdef __APPLE__
# include <SDL_ttf/SDL_ttf.h>
# include <SDL_image/SDL_image.h>
# include <SDL2/SDL_ttf.h>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this will be correct once the SDL2 version's running properly, it's not correct now, though I guess it probably doesn't matter whether it's correct since you can't compile it either way (and yeah, it wouldn't take much to convince me to merge despite this issue, just think it needs addressing first)

@@ -1,4 +1,5 @@
#include <string>
#include <unistd.h>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes a failed compile on windows, since there's no unistd.h there and we do in fact compile this file on windows--not a major problem though

Copy link
Owner

@Putnam3145 Putnam3145 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one's a bit of a weird one--I'm going to merge it, then I'll probably spend a while building stuff on top, modifying etc.--normally I'd request changes for a regular open-source PR, but the whole pipeline is so bespoke and strange that I figure it's far more painless if I fix anything that the contributor couldn't possibly know is broken rather than asking them to fix it. Unless something comes up in the next 24 hours, though, I'm merging it.

weigert and others added 5 commits July 6, 2023 13:29
@weigert
Copy link
Author

weigert commented Jul 6, 2023

Note that I rebased the fork branch on your new master.

That would be awesome. I am still very interested in contributing if there are small projects, ideas or quality of life things which can be added in a decentralized manner. I think getting it to compile is for sure the first step!

I think a number of additional, small changes that can be done in a decentralized manner could greatly improve the potential for community contributions (and contributor sanity):

  • Like @myk002 stated, adding some GitHub actions for automatic compilation checking would be awesome!
  • Run a linter with a fixed style and enforce it through GitHub actions
  • Some small directory structuring, so that utility code, widgeting code, etc. are cleanly separated for legibility
  • Making this library dynamically linked, so that it can be 100% externally compiled and even replaced in the game for testing purposes
  • Potentially making widgets dynamically loaded classes, with appropriate hooks so that custom windows can be spawned (afaik this is currently possible w. dfhack, but making this easier to interface with would be nice). UX improvements could then be shipped much more easily by modders, for a better "see what works" approach

Let me know what you think. I am happy to contribute to any of these ideas (or others which you might have).

@Putnam3145
Copy link
Owner

I do in fact want widgets to be dynamically loaded at some point, either loaded from raws or some more "standard" scripting approach.

The merge hasn't happened because I'm still not sure when it should happen--like, if it actually ends up as shipped code, or if it's a sure thing, et cetera. There's also the contributors thing--MIT license means we need to list the contributors somewhere. There's a TXT file containing all the credits that it reads from that we might be able to add to, or maybe a CONTRIBUTORS file somewhere. It's still a bit up in the air. That all happened "within the next 24 hours", of course. I forgot to mention it, whoops.

@weigert
Copy link
Author

weigert commented Aug 1, 2023

Hi @Putnam3145. If and when you merge any pull request is of course at your full discretion. Should you make a decision on what contributions will look like, and if they will be shipped or not, I will be happy to contribute under those parameters. Until then, I guess there is no rush. Cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants