-
Notifications
You must be signed in to change notification settings - Fork 107
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
Support 64-bit linux builds #31
Support 64-bit linux builds #31
Conversation
I definitely appreciate the help! Although we're pushing forward with the engine, we don't have the man-power to keep the different platforms all running smoothly as we go. Naturally Linux doesn't get the love it needs so I appreciate any help you can offer. Any help you can offer with performance for the new code is also welcome. I've added the tag for the pull request to be tracked. |
Perhaps we should try to get some regression-test-sufficiency in place for these more esoteric platforms? I think that Github Actions, or TravisCI, or CirlceCI can make sure we know if we break the build. Do you have a preference for which? Happy to include it when I add the dockerized build of 32 bit linux. |
I would love to do that. I know I looked at TravisCI once but there's just too much that is outside my knowledge to set it up correctly. I would need someone's help ;) Of course, the bigger problem isn't knowing that it's broken, it's finding someone to fix it. |
cd2a8ba
to
9399642
Compare
Dropping draft status, as we're building and running! I rebased this branch on the 32-bit branch, so perhaps start by giving #32 a review? :) |
@@ -269,7 +269,7 @@ U32 GuiSpriteCtrl::constructBitmapArray() | |||
return mBitmapArrayRects.size(); | |||
} | |||
|
|||
Point2I& GuiSpriteCtrl::constrain(Point2I &point, bool grow) | |||
Point2I GuiSpriteCtrl::constrain(Point2I &point, bool grow) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a compiler optimization or is it considered better practice to return an object rather than a reference to an object? I assumed that returning the reference would avoid making a copy of the object and would therefore be faster, but I would be interested to know if that isn't true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Your intuition is correct that returning a reference, ie just a memory address that can fit on a register, is faster in principle, but C++11 will (functionally, this is tea leave reading of the rvalue/lvalue laden blog posts and docs) only let you return a reference to an object known to the caller.
That is, because this function makes a new Point2I
(on lines 283 and 287), you must return a whole Point2I
object since the caller can't know about it ahead of time. I think this kinda makes sense? You can't return a reference to an object that is local to this function because when the function is done executing, poof, the local storage is gone and the reference you handed out points into the void.
You're correct that this is unfortunately not as performant as just handing around memory addresses, but, since we're not garbage collected, we can't make assumptions about validity of references without these rules. There is a saving grace, here, in that most computers can still pass around classes that don't own data on the heap very quickly. In the event it's not fast enough, or class has a handle to a buffer in memory that it must preserve ownership of, you can probably implement the move constructor and use move assignment to account for things byte-by-byte yourself via pilfering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good blog post. Apparently I was still operating in the blissful days of C++03 in this area. I don't believe I will start using move at this time, but considering how many Rects, Points, Vectors, and Colors we copy around, it would probably be a significant improvement to do so at some point.
As far as returning a reference to a local, that makes sense. What doesn't make sense is that it worked. I can only guess that my compiler (Microsoft has their own C++ compiler) was fixing it for me and GCC was not. At any rate, I'll move ahead with reading through this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll probably take a look at windows build here in a week or so,we can do CI builds of that too :)
I'd encourage you to look at the 32-bit PR first, this one is based on that one and this change comes from that branch. I kinda stacked my PRs since I'm touching a lot of stuff in multiple places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! I started on x64 support, noticed that 32 bit was busted so posted this draft then swapped over there, then came back here to rebase on top of that, then went to do the CI work. Sorry for the chaos!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! I prefer chaos over silence ;)
Hi and happy Hacktoberfest! I've done a few compiler-related changes over the years and I thought I'd circle back here and see how things are going. The early access release for the rocket edition looks pretty cool and I'm excited to see T2D is still alive!
To be upfront: I do not expect that this will merge. I am definitely making some bad calls here in the quest to just-get-building-quickly and I wholeheartedly believe there's substantial work to be done once we do get sandbox running. Moreover, I am not testing this on any other platforms yet, so I am not sure if, for instance, I'm breaking win32 anywhere (or even Linux32).
Linux build is pretty sad. 32-bit only in 2021 is definitely not up-to-date and I'm also a little worried that the documentation on building 32 bit linux is weaker than it could be. To that end, I'm separately working on a build-in-docker solution that I think might be much more likely to merge as-is, watch out for that in a week or so.
Happy to discuss inline in the diff any nuances I'm omitting here, but the high level bullets:
engine/source/gui/guiDefaultControlRender.cc
, the functionrenderSizableBorderedTexture()
takes a number ofRectI
references as parameters, but the sole caller of the function is building thoseRectI
s inline in the function call, which GCC doesn't like because the function gets a reference to a temporary, not a local. In this particular case, I was able to get it working by peeling theRectI
s into locals just above the function call, but in general there's probably a more Torquey solution for the changes I made to the GUI code to get things building.Offset()
macro, as recent GCC does not like trying to figure out the offset of a field in a class, only structs. The original implementation ofOffset()
doesn't work on 64 bit, and for the replacement I tried to use theoffsetof()
builtin. That "compiles" but it spews an entirely reasonable warning message about this being a GNU implementation of an undefined behavior in the standard, and as such should not be trusted. I also silenced the warning in the makefile's CC flags, because we use it a lot.This is not currently running, partly because the math assembly files do not work on x64 (I am tempted to find a proper C++ implementation of those functions, because I'm optimistic that we're not saving that many cycles these days), and partly because I still have work to figure out in the linker configs.
Publishing now, with edits enabled, so we can collaborate, discuss this concept, and see if it's worth further pursuit.
Of course, I'm doing this as a pure driveby as a former contributor, and you guys are welcome to decline if it's not in line with your current goals. If you happen to be interested in other drivebys this Hacktoberfest, you can find out how to invite participation for the project over at https://hacktoberfest.digitalocean.com/resources/maintainers - it's as simple as adding a label to the repo. If you do not decide to opt in globally, but you consider this work to be valuable to you, I would ask that you at least consider adding the
hacktoberfest-accepted
label to this PR, so that this work can be tracked by the festival.Cheers and congrats on an awesome early access!