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

Split engine to subsystems #4304

Open
DarkWeird opened this issue Dec 7, 2020 · 11 comments
Open

Split engine to subsystems #4304

DarkWeird opened this issue Dec 7, 2020 · 11 comments
Assignees
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature

Comments

@DarkWeird
Copy link
Contributor

DarkWeird commented Dec 7, 2020

Extraction subsystems should:

  1. simplify codebase via extracting common components
  2. make some components optional/replaceable. e.g:
    Optional: Telemetry and Discord not needs for server build
    Replaceable: AudioLwjgl and AudioNull subsystems.
  3. provide native library support for modules. e.g:
    Kallisti -> Jnlua
    CoreRendering -> GraphicsLwjgl

Extraction subsystems shouldn't:

  1. Replace Modules
  2. Downloads from server or internet
  3. Loads dynamically

Draft extraction scheme:

Disclaimer: some listed subsystem is just library - haven't EngineSubsystem implementation. but i cannot place it at libs directory, because libs directory used for external libraries (external repo)

Subscribables is timed subsystem. while AutoConfigs will not extracted and configs not migrated to autoconfigs

Required Features:

  1. feat: Reviving of "UI for editing arbitrary types and AutoConfigs" #4244 - for autoconfig. it ideally config system for subsystem ecosystem. needs 7 and 2.
  2. Module -> Subsystem dependency.
    Partial current solution for 'Library' subsystems - engine provides them as gradle's api dependency (formal it is engine dependency)

Extractions

  1. DiscordRPC subsystem feature(subsystems): extract DiscordRPCSubSystem #4233
  2. TypeHandlerLibrary feat(subsystems): extract TypeHandlerLibrary. #4255
  3. ✅ Add generic Serializer for TypeHandlerLibrary feat(TypeHandlerLibrary): Add generic Serializer for TypeHandlerLibrary. #4324
  4. extract EntitySystem. ( requires ✅ feature(subsystem-prepare): extract Network and R&R code from EventSystemImpl #4565 for pure events and ✅ feat(TypeHandlerLibrary): Add generic Serializer for TypeHandlerLibrary. #4324 for serialize components)
  5. protobuf serialize over GenericSerilaizer(3)
  6. network subsystem - needs 4
  7. GsonSerializer feat(subsystems): Extract GsonSerializer #4270 - needs 2.( follow-ups - migrate engine's code from pure Gson to GsonSerializer) Outdated with feat(TypeHandlerLibrary): Add generic Serializer for TypeHandlerLibrary. #4324
  8. extractTypeWidgetLibrary -- but first needs to merge feat: Reviving of "UI for editing arbitrary types and AutoConfigs" #4244
  9. extract AutoConfigs - needs 7.
  10. extract Audio(Null|API|Lwjgl) subsystems. required .
  11. extract Graphics - required AutoConfigs as subsystem and migrated Rendering configs to AutoConfig.
  12. extract Record&Replace - highly integrated in engine. needs refactor WorldStorage to make it extendable.
  13. ???
  14. PROFIT!
@DarkWeird DarkWeird added Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature labels Dec 7, 2020
@DarkWeird DarkWeird self-assigned this Dec 7, 2020
@jdrueckert
Copy link
Member

jdrueckert commented Dec 8, 2020

In Discord @DarkWeird mentioned the following configs that will need to be migrated to AutoConfig:

  • audio
  • system
  • player
  • permission
  • input
  • binds
  • rendering
  • defaultModSelection
  • worldGeneration
  • moduleConfigs
  • network
  • security
  • nuiEditor
  • selectModulesConfig
  • identityStorageService
  • telemetryConfig
  • universeConfig
  • webBrowserConfig

@Cervator
Copy link
Member

Cervator commented Jan 1, 2021

Small note:

Optional: Telemetry and Discord not needs for server build

Discord: correct. Telemetry: want! Great way to send server logs and performance. There are client specific bits to the telemetry system though, like blocks broken and such. That part we could certainly skip, but then I'm not sure if that would enable by default 🤔

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Oct 25, 2021

Investigation of possible to extract AutoConifg as Subsystem:

  1. Context should be extracted (feat(extract-subsystems): extract Context, @In, InjectionHelper and @Share to Subsystem #4930) - AutoConfig injects Autoconfigs to context and have @In Injects
  2. AutoConfigScreen - takes ModuleManager(engine) and CoreScreenLayer(engine)
    3.AutoConfigWidgetFactory - takes ModuleManager(engine) and TranslationSystem(engine)
  3. ColorConstraintWidgetFactory - takes CieCamColors(engine, used for ColorWidget and PlayerConfig also useless ColorHueAnimator), TextureUtil(engine, for color uri) and Texture(engine, loading asset)
  4. LocaleConstraintWidgetFactory - takes SimpleUri(engine, can be replaced with ResourceUrn(gestalt)) and TranslationSystem(engine)
  5. AutoConfig - takes SimpleUri(engine, can be replaced with ResourceUrn(gestalt)
  6. AutoConfigManager - takes Context(engine, subsystems:Context on the way), SimpleUri(engine), PathManager(engine, for save and loading), ModuleManager(engine, can be replaced with directly ModuleEnvironment(gestalt-module)) and ReflectionUtil(engine, resolve SimpleUri from config class and ModuleEnvironment(module-gestalt)))

Possible bonus. if we extract saving and loading from AutoConfigManager then per-game-saves will easier.(PathManager)

Think to do:

  1. extract Context (feat(extract-subsystems): extract Context, @In, InjectionHelper and @Share to Subsystem #4930)
  2. extract TranslationSystem (there have Translator(nui). but we needs getProject method.. which don't in traslator) or do TODO there
  3. replaces SimpleUri with ResourceUrn (partially)
  4. idk what to do with CieCamColors(engine)
  5. idk what to do with Texture and TextureUtil
  6. Idk where move ReflectionUtil.getFullyQualifiedSimpleUriFor(engine). because AutoConfig is single user... but Yet Another ReflectionUtl.... (we have it in TS, in NUI)

@keturn
Copy link
Member

keturn commented Nov 12, 2021

I was having trouble understanding #4930, but maybe re-reading the last comment here is helping.

I was thinking: Why bother extracting Context? Perhaps it is so Subsystems can use it, but, no, Subsystems do depend on Engine — they must, if for no other reason than the EngineSubsystem interface is defined in Engine. That means Subsystems may use a Context class defined in Engine too.

Looking at this, it seems it's because you want to extract AutoConfig as a library that has no Engine dependency?

It would help me keep things straight if we use different words for Subsystems vs non-Engine libraries. If the only obstacle to calling those “libraries” or “libs” is that we currently have the /libs/ directory doing a different job in our Gradle configuration, we can surely move or rename that.

I am also not totally sold on the utility of extracting libs to their own gradle sub-projects. It does carry some maintenance cost with it: for example, the recent case where we neglected to run tests on TypeHandlerLibrary for many months because we no longer invoke tests in the same way in all our sub-projects. On the other hand, it does add some constraints that help keep things more organized by preventing circular dependencies between packages.

@DarkWeird
Copy link
Contributor Author

DarkWeird commented Nov 13, 2021

I don't like current TS's engine:

  1. A big mess of classes and packages, which connected somehow, leaked interfaces
  2. Poor submodule level documentation
  3. Mix from bootstrap code(states, subsystems, TerasologyEngine...) and engine module.
  4. Many conditional code, which don't using 99% time (e.g. telemetry, record and replay, headless impls(server or mte users)

Then i suggest:

  1. Split to small code modules with strict describes, what this code module does.
  2. Document subsystem and parts as code module (see THL. It pretty. Isn't it?)
  3. Separate engine module. Bootstrap should be splitted by (1)
  4. Conditional code should be as separate code module.
    (Summary:
    made enterprise level splitting (api/impl):
    Like androids apps, like springs apps. Like bestpractics about code modules sinse ejb)

I think about those categories now:

  1. External lib (/libs/) - library, which can be used everywhere. For example: You can clone guava there
  2. EngineSubsystem (/subsystems/) - engine subsystem implementation.
    2.1. provides functionality for modules (there should be like jnlua bindings, don't should break sandbox, cannot be downloaded, related modules cannot be activated without this subsystem in game installation)
    2.2. provide functionality for facades (there DiscordRPC, using engine data, passtru it to discord, can be using by facade only. Circular dep otherwise)
  3. Engine-part or engine-lib - provides functionality or api(!) for other subsystem(or modules, or facades)
    3.1 candidate for being lib - there THL, it almost ready for being gestalt-serialization.
    3.2. api - interfaces which can be used by subsystem or engine(bootstrap part) - there Context(implementation in engine), there will NetworkAPI (provides server/client interfaces, implementation will at NetworkNetty)

P.S.
AutoConfig should provide config functionality for:

  1. Modules (not implemented yet, except engine module.it not ordinal module)
  2. Engine Subsystems (like lwjgl or discord)

We cannot provide functionality for non-extracted subsystem and engine module...
Because autoconfig using context.
But we can extract Context-api and then extract autoconfigs!

Another example:
We cannot extract rendering.
We should extract api and impl...
Because rendering using from bootstrap code(nuimanager)
Then. Api should not use engine(circular dep)
.

MTE example:
MTE deeply using engines parts(runs engines and other infrastructure)
We can extract it as subsystem (code module) and use it at engine.test(source set), facades, modules.tests.
I think ideally place :3

You can provide you naming for /subsystem/

@keturn
Copy link
Member

keturn commented Nov 13, 2021

I think these are good categorizations. 👍

Which of them want to be their own gradle-(sub)projects?

Advantages of being a separate gradle (sub)project:

  • Each builds a separate jar, which lets downstream users choose whether to include it in their dependencies or not.
  • Projects must explicitly declare their dependencies in build.gradle, which should force us to structure things so connections are clearer and not circular.
  • A bit easier / faster to say “rebuild only this part” or “run tests for only this part.”

Disadvantages of being separate:

  • Each has its own build.gradle to maintain.
  • More difficult to navigate the HTML Javadoc.

(There are, of course, ways to mitigate these disadvantages, but it adds a bit of work to do so.)

If an EngineSubsystem is optional or replaceable, so that a particular Facade may choose to build an application without it, I think that provides a good reason for wanting a separate jar.

On the other hand, a library such as THL will necessarily be a dependency for everything; no one will ever get the engine.jar without also getting the THL.jar.

Another thought, now that Terasology requires Java 11: Are Java Modules an alternative (better? worse?) way of describing the connections between these different packages and enforcing the API encapsulations we want for them? (If that is a can of worms that is too messy for now, please feel free to say so.)

@DarkWeird
Copy link
Contributor Author

Each has its own build.gradle to maintain

Yeah. I think about version catalog :3

More difficult to navigate the HTML Javadoc

Ehm. We have this?
I think somewhere exists solution with html javadocs over several java libs.

On the other hand, a library such as THL will necessarily be a dependency for everything; no one will ever get the engine.jar without also getting the THL.jar.

There another reasons:

  1. Smaller engine part > easier maintain, stricter dep describing.
  2. (Yeah it can be convert to java module easier :p)

Another thought, now that Terasology requires Java 11: Are Java Modules an alternative ...
I have some thoughts about this, but it is future:

  1. Replaces @API and our sandbox/security
  2. Activate more aggressive optimization
  3. Seems easier tera module describes.(instead package module and jar module
    Cons:
  4. Java module have problems with unit tests yet(unit tests cannot access to internal packages)
  5. Needs support from gestalt side

@keturn
Copy link
Member

keturn commented Nov 15, 2021

Using Java Modules as gestalt Modules is a whole topic in itself. For now, I was wondering about these library-ish things, whether java modules' ability to define requirements and exports would help in this… but taking another look at that now, I think jars can only contain one module, and each gradle SourceSet can make one jar/module. That means this is not an alternative we could do instead of splitting these into different source directories— it's something additional we could do after splitting them up.

@keturn
Copy link
Member

keturn commented Nov 15, 2021

Mainly though, the "smaller engine part > easier maintenance" is what I am slow to understand. Why is org/terasology/engine/registry/*.java easier to maintain if it is in /subsystems/Context/src/main instead of /engine/src/main?

You mention dependencies, and it does make it so if you're editing something there you can't just auto-import something from org.terasology.engine.world without thinking about it; you have to pause and consider whether to manually add that dependency.

That is worth something, but I keep wondering if I have missed something else, because it doesn't quite sell me by itself.

@keturn
Copy link
Member

keturn commented Nov 15, 2021

On Javadoc: We do have Javadoc published on jenkins, if nowhere else. I think the build generates javadoc for everything, but the Jenkins built-in javadoc integration only publishes one javadoc directory for the project, so we have it pointing at /engine currently.

@DarkWeird
Copy link
Contributor Author

Mainly though, the "smaller engine part > easier maintenance" is what I am slow to understand. Why is org/terasology/engine/registry/*.java easier to maintain if it is in /subsystems/Context/src/main instead of /engine/src/main?

Try to locate all parts of rendering.
Try to locate all parts of any serialization.
Try to locate all parts of health and destruction.
Try to locate all parts of networking.

They placed in different packages.
I try isolate one functionality in one place.

Context required to extract for another subsystems.
(Also... Those gradle modules should be looks fine at dependency graph visualization)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Architecture Requests, Issues and Changes related to software architecture, programming patterns, etc. Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

No branches or pull requests

5 participants