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

Allow for platform Thread implementation override #52734

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

lucypero
Copy link
Contributor

@lucypero lucypero commented Sep 16, 2021

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Looks good to me, besides the formatting issues.

core/os/thread.cpp Outdated Show resolved Hide resolved
core/os/thread.h Outdated Show resolved Hide resolved
core/os/thread.h Outdated Show resolved Hide resolved
@akien-mga akien-mga changed the title Allow for platform thread override Allow for platform Thread implementation override Sep 16, 2021
@lucypero
Copy link
Contributor Author

Fixed formatting and added comment.

core/os/thread.cpp Outdated Show resolved Hide resolved
core/os/thread.h Outdated Show resolved Hide resolved
@Listwon
Copy link
Contributor

Listwon commented Sep 16, 2021

@akien-mga isn't this PR against the contribution best practices? Reduz mentioned them recently on Twitter https://docs.godotengine.org/en/stable/community/contributing/best_practices_for_engine_contributors.html

These changes are useful only in very special cases like console porting. Dangling #include "platform_thread.h" without any stub is a bit misleading. If you are porting engine to special platform, you can handle these tiny additions on your private repository.

@akien-mga
Copy link
Member

Seems good to me, please squash the commits into one after final edits (see PR workflow for instructions).

@akien-mga
Copy link
Member

These changes are useful only in very special cases like console porting. Dangling #include "platform_thread.h" without any stub is a bit misleading. If you are porting engine to special platform, you can handle these tiny additions on your private repository.

I don't think console porting is a "very special case". It's something a significant number of users need to do or get access to to publish their games on consoles. Adding a documented opt-in preprocessor define for that doesn't strike me as particularly problematic.

For console porters, or people porting to some niche platforms, that means that they don't need to do a hard fork of the engine to add a platform port (instead, they can just drop their platform code in the platforms folder and compile). Whether that's possible in practice, I don't know, but that's how at least some porters used to work significantly in the past.

I don't think this PR goes against the contribution best practices. It doesn't modify everyone's Thread implementation for a niche use case - it adds an entry point to allow defining a custom one, which is not even compiled if not defined.

@Listwon
Copy link
Contributor

Listwon commented Sep 16, 2021

In that case "platform_thread.h" doesn't make sense anyway, should be stubbed somehow with some informative comments, thread class stubs, asserts, defines. Previously it was virtual class implemented per platform and now it's #include of nonexistent header in root path.

@reduz
Copy link
Member

reduz commented Sep 16, 2021

To clarify from my side. The general policy is to integrate what is needed by the community, this includes companies using the engine, even those that need this to make ports to consoles easier to maintain.

If something is not wanted by the community, or it is not possible to add it due to legal reasons (like this case), at least the project will always have a compromise to make hooks available for this to happen and, thus, minimize the amount of forked codebase needed.

@reduz
Copy link
Member

reduz commented Sep 16, 2021

I would still add a comment in that line of code that reads like "Overriding the platform implementation is required in some proprietary platforms".

@akien-mga
Copy link
Member

In that case "platform_thread.h" doesn't make sense anyway, should be stubbed somehow with some informative comments, thread class stubs, asserts, defines. Previously it was virtual class implemented per platform and now it's #include of nonexistent header in root path.

I don't think it's necessary to make a stub for this. Custom implementation just need to match the interface of core/os/thread.h which they replace.

However, it's true that enforcing a "platform_thread.h" header which may or may not be in the include path is not the most convenient.
It could be #include PLATFORM_CUSTOM_THREAD_H which can be #define'd to the platform's relevant file path in platform_config.h.

@reduz
Copy link
Member

reduz commented Sep 16, 2021

Agreed, just a single PLATFORM_CUSTOM_THREAD_H define that points to the include is probably enough.

@lucypero
Copy link
Contributor Author

Replaced PLATFORM_THREAD_OVERRIDE with PLATFORM_CUSTOM_THREAD_H

@akien-mga
Copy link
Member

Looks great. Just remember to update the #endif // PLATFORM_THREAD_OVERRIDE comments in both files, and squash the commits.

@lucypero
Copy link
Contributor Author

Looks great. Just remember to update the #endif // PLATFORM_THREAD_OVERRIDE comments in both files, and squash the commits.

Done.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 16, 2021
@akien-mga akien-mga merged commit 191c34e into godotengine:master Sep 16, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 21, 2021
@lucypero lucypero deleted the thread_override_master branch September 25, 2021 00:08
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.

5 participants