feature: migrate to gestaltv7 by pollend · Pull Request #4593 · MovingBlocks/Terasology

pollend · 2021-03-21T18:53:07Z

Here is the initial migration to gestalt-v7.

Changes

update gestalt package
- org.terasology.gestalt.assets.management.AssetManager
- org.terasology.gestalt.naming.Name
- org.terasology.gestalt.assets.AssetType;
- org.terasology.gestalt.assets.management.AssetManager;
- org.terasology.gestalt.assets.module.ModuleAwareAssetTypeManager;
- org.terasology.gestalt.assets.module.ModuleAwareAssetTypeManagerImpl;
- org.terasology.gestalt.module.ModuleEnvironment;
- org.terasology.gestalt.module.ModuleRegistry;
- org.terasology.gestalt.module.dependencyresolution.DependencyResolver;
- org.terasology.gestalt.module.dependencyresolution.ResolutionResult;

Issues

something jmh-related preventing running from IntelliJ?
tidy use of google() repository
chore: remove DebugPropertySystem #4610 Crash on F11 (DebugControlSystem failure to get MouseDevice)
migrate the modules: many have branches under the name chore/gestalt-v7-migration

Module Changes

feature: gestalt v7 migration Terasology/CoreRendering#36

pollend · 2021-03-21T18:53:34Z

any clue why this isn't working

Terasology/engine/src/main/java/org/terasology/engine/core/module/ModuleManagerImpl.java:105:17
java: cannot access com.github.zafarkhaja.semver.Version
  class file for com.github.zafarkhaja.semver.Version not found

pollend · 2021-03-21T18:55:06Z

+    // umm need for android dependencies
+    repositories {
+        google()
+    }
+


I have these littered around in a few places any clue how to correct these imports. looks like we are dependencies on some android dependencies ?!

keturn · 2021-03-21T23:20:52Z

Adding it to terasology-repositories in build-logic would normally be sufficient for adding a repository, but since it's a dependency of gestalt and build-logic uses gestalt itself, I think it'll also need to be in buildscript { repository { } } sections. That includes the root build.gradle as well as the templates/build.gradle for modules.

(Yeah, I'm starting to doubt the wisdom of making build-logic depend on gestalt. Or wonder if there's some way for build-logic to be responsible for its own repositories so it doesn't leak out to the other buildscript sections.)

keturn · 2021-03-22T00:08:34Z

I was just going to go look at ModuleManager and discovered that it's an interface again, undoing #4539 (which undid #1665).

did you discover a reason for that to be an interface that we missed when I submitted #4539?

pollend · 2021-03-22T04:23:27Z

I was just going to go look at ModuleManager and discovered that it's an interface again, undoing #4539 (which undid #1665).

did you discover a reason for that to be an interface that we missed when I submitted #4539?

I was referencing @DarkWeird older PR. I'll undo that change.

pollend · 2021-03-22T05:03:23Z

I'm having some strange rendering problems where you only see the last dropped item. not sure what is causing it and how to resolve the problem.

DarkWeird · 2021-03-22T08:33:31Z

I'm having some strange rendering problems where you only see the last dropped item. not sure what is causing it and how to resolve the problem.

Dropped items has same LocationComponent because using default CopyStrategy because nui-reflect haven't reflections.cache (and Reflections didn't fetch nui-reflect at runtime, because it not part of org.terasology.nui package)

Probably it can be resolved with java 8 (reflections have several glitches at java 11... classloadersss...)
Or provide reflections.cache from nui-reflect`

keturn · 2021-03-22T19:11:58Z

If this is using MovingBlocks/TeraNUI#46, does that mean we don't need to do the change to the NUI namespaces in MovingBlocks/TeraNUI#27 at the same time?

edit: oh, wait, that's what DW's comment just prior to this one was about, things failing because a nui package is not part of org.terasology.nui.

DarkWeird · 2021-03-22T20:56:52Z

If this is using MovingBlocks/TeraNUI#46, does that mean we don't need to do the change to the NUI namespaces in MovingBlocks/TeraNUI#27 at the same time?

edit: oh, wait, that's what DW's comment just prior to this one was about, things failing because a nui package is not part of org.terasology.nui.

I think use BSA's PR firsts, then provide support in nui-gestalv7 module is better then syntetic module

keturn · 2021-03-24T02:29:07Z

Sounds like you've been learning about what works and what doesn't.

We are going to end up with a PR that changes a zillion import statements; with changing the packages of things like Name, ResourceUrn, and API, that's unavoidable. Are there any pieces of this we can pull out and do in more easy-to-review PRs?

The thing with engine/src/jmh seems like it's probably one, since it moves a bunch of stuff but I'm not really sure what that's about.

Are there other fixes that would also apply to the current code even without the gestalt change? e.g. the camera position?

keturn · 2021-03-29T19:46:56Z

I haven't tried it out yet, but I strongly suspect the thing you described with jmh being pulled in to the reflections is from this:

Terasology/engine/build.gradle

Lines 130 to 134 in 21d13a3

    
           task cacheReflections { 
        
               description = 'Caches reflection output to make regular startup faster. May go stale and need cleanup at times.' 
        
               inputs.files sourceSets.main.output.classesDirs, 
        
                       // getClassesDir from all sourceSets (for any jvm (seems) language) 
        
                       configurations."${sourceSets.main.runtimeClasspathConfigurationName}"

note how the comment says "all sourceSets." Which might be useful if we had parts of engine written in Kotlin or Scala, but is probably not helping us as it is.

And yes, I'm still hoping to roll back the jmh part of this PR. Not just for size; also because I think in general we want to move in the direction of "move things from engine-test/src/test to engine/src/test" instead of "move things from engine to engine-test." Admittedly I am really not at all clear on where the jmh sources fall in that or how they are used.

pollend · 2021-03-29T19:48:44Z

I haven't tried it out yet, but I strongly suspect the thing you described with jmh being pulled in to the reflections is from this:

Terasology/engine/build.gradle

Lines 130 to 134 in 21d13a3

task cacheReflections {

description = 'Caches reflection output to make regular startup faster. May go stale and need cleanup at times.'

inputs.files sourceSets.main.output.classesDirs,

// getClassesDir from all sourceSets (for any jvm (seems) language)

configurations."${sourceSets.main.runtimeClasspathConfigurationName}"

note how the comment says "all sourceSets." Which might be useful if we had parts of engine written in Kotlin or Scala, but is probably not helping us as it is.

And yes, I'm still hoping to roll back the jmh part of this PR. Not just for size; also because I think in general we want to move in the direction of "move things from engine-test/src/test to engine/src/test" instead of "move things from engine to engine-test." Admittedly I am really not at all clear on where the jmh sources fall in that or how they are used.

yep, go ahead and revert the jmh part of this PR.

keturn · 2021-04-02T01:02:15Z

Getting two NPEs after I save a screen shot. One of them is coming from NUISkinEditorScreen, which is very puzzling given that I don't have that screen open nor have I used it this run at all:

[main] ERROR o.t.engine.core.TerasologyEngine - Uncaught exception, attempting clean game shutdown
com.google.common.base.VerifyException: Module "assets" not found in current module environment.
	at com.google.common.base.Verify.verify(Verify.java:124)
	at com.google.common.base.Verify.verifyNotNull(Verify.java:500)
	at org.terasology.engine.rendering.nui.editor.layers.AbstractEditorScreen.getPath(AbstractEditorScreen.java:336)
	at org.terasology.engine.rendering.nui.editor.layers.NUISkinEditorScreen.setSelectedAssetPath(NUISkinEditorScreen.java:510)
	at org.terasology.engine.rendering.nui.editor.layers.AbstractEditorScreen.loadAutosave(AbstractEditorScreen.java:299)
	at org.terasology.engine.rendering.nui.editor.layers.AbstractEditorScreen.onDraw(AbstractEditorScreen.java:157)
	at org.terasology.nui.canvas.CanvasImpl.drawStyledWidget(CanvasImpl.java:442)
	at org.terasology.nui.canvas.CanvasImpl.drawWidget(CanvasImpl.java:428)
	at org.terasology.engine.rendering.nui.internal.NUIManagerInternal.render(NUIManagerInternal.java:570)
	at org.terasology.engine.core.modes.StateIngame.renderUserInterface(StateIngame.java:231)
	at org.terasology.engine.core.modes.StateIngame.render(StateIngame.java:215)
	at org.terasology.engine.core.subsystem.lwjgl.LwjglGraphics.postUpdate(LwjglGraphics.java:89)
	at org.terasology.engine.core.TerasologyEngine.tick(TerasologyEngine.java:497)

keturn · 2021-04-02T01:05:21Z

The other is from DebugControlSystem:

[main] ERROR o.t.e.e.e.internal.EventSystemImpl - Failed to invoke event
java.lang.NullPointerException: null
	at org.terasology.engine.logic.players.DebugControlSystem.onKeyDown(DebugControlSystem.java:133)
	at org.terasology.engine.logic.players.DebugControlSystemMethodAccess.invoke(Unknown Source)
	at org.terasology.engine.entitySystem.event.internal.EventSystemImpl$ByteCodeEventHandlerInfo.invoke(EventSystemImpl.java:389)
	at org.terasology.engine.entitySystem.event.internal.EventSystemImpl.sendConsumableEvent(EventSystemImpl.java:268)
	at org.terasology.engine.entitySystem.event.internal.EventSystemImpl.send(EventSystemImpl.java:247)
	at org.terasology.engine.core.bootstrap.eventSystem.AbstractEventSystemDecorator.send(AbstractEventSystemDecorator.java:67)
	at org.terasology.engine.network.NetworkEventSystemDecorator.send(NetworkEventSystemDecorator.java:54)
	at org.terasology.engine.core.bootstrap.eventSystem.AbstractEventSystemDecorator.send(AbstractEventSystemDecorator.java:67)
	at org.terasology.engine.recording.RecordingEventSystemDecorator.send(RecordingEventSystemDecorator.java:34)
	at org.terasology.engine.entitySystem.entity.internal.BaseEntityRef.send(BaseEntityRef.java:205)
	at org.terasology.engine.input.InputSystem.send(InputSystem.java:525)
	at org.terasology.engine.input.InputSystem.sendKeyEvent(InputSystem.java:454)
	at org.terasology.engine.input.InputSystem.processKeyboardInput(InputSystem.java:383)
	at org.terasology.engine.input.InputSystem.update(InputSystem.java:131)
	at org.terasology.engine.core.modes.StateIngame.handleInput(StateIngame.java:192)
	at org.terasology.engine.core.subsystem.lwjgl.LwjglInput.postUpdate(LwjglInput.java:39)
	at org.terasology.engine.core.TerasologyEngine.tick(TerasologyEngine.java:497)

[I really wish java or github or something in this toolchain would inline sources in tracebacks.]

We're not on Java 14 yet so we don't have useful NPE messages, but that line is

Terasology/engine/src/main/java/org/terasology/engine/logic/players/DebugControlSystem.java

Line 133 in 7143a6c

mouseDevice.setGrabbed(mouseGrabbed);

so I guess it's probably mouseDevice that is null???

keturn · 2021-04-02T01:39:44Z

ERROR o.t.engine.registry.InjectionHelper - BlockCommands wanted WorldProviderCoreImpl injected but CoreRegistry has none.
ERROR o.t.engine.registry.InjectionHelper - DebugControlSystem wanted MouseDevice injected but CoreRegistry has none.

* feature: add subsystem to modulemanager * bugfix: correct crash with ModuleManager

…stalt-v7

keturn

I think like we have some things to clarify about the fundamental the goals of subsystem extraction.

keturn · 2021-04-04T17:04:54Z

-    private Module loadEngineModule(List<Class<?>> classesOnClasspathsToAddToEngine) {
-        // FIXME: is `classes…toAddToEngine` gone? Did we ever use it in the first place?


Did you check for classesOnClasspathsToAddToEngine usage? I know it's not usually used, so maybe this is a non-issue, but I had a vague memory about it being used somewhere...

keturn · 2021-04-04T17:06:25Z

-//    private void enrichReflectionsWithSubsystems(Module engineModule) {
-//        Serializer serializer = new XmlSerializer();


why doesn't the gestalt-v7 branch need an enrichReflectionsWithSubsystems method? v7 hasn't done away with Reflections yet…

keturn · 2021-04-04T17:14:22Z

+    private  void createDependency(Module parent, Module child) {
+        DependencyInfo dependencyInfo = new DependencyInfo();
+        dependencyInfo.setId(child.getId());
+        dependencyInfo.setMinVersion(child.getVersion());
+        dependencyInfo.setMaxVersion(child.getVersion().getNextPatchVersion());
+        parent.getMetadata().getDependencies().add(dependencyInfo);


👍 makes sense to have a function for this.

TODO: refactor ensureModulesDependOnEngine method (just below this one) to use this.

FIXME: note 20546f1 — getNextPatchVersion may break for this purpose when base version is a SNAPSHOT.

keturn · 2021-04-04T17:17:22Z

+
+        registry.add(nui);
+        registry.add(engineModule);
+        registry.add(discordSubsystem);


hrmmm. Not sure about adding hardcoded references to subsystems in engine's ModuleManager.

The point of extracting subsystems—as I understand it—is to make it so the engine doesn't always need to depend on them. i.e. a facade should be able to start an engine that doesn't have a DiscordRPC subsystem at all.

yea, at the moment its a hack to get the game to run. System is registered but it can't resolve autoconfig when it tries to scan with environment.

keturn · 2021-04-04T17:20:26Z


        setupSandbox();
-        loadEnvironment(Sets.newHashSet(engineModule), true);
+        loadEnvironment(Sets.newHashSet(engineModule, nui, discordSubsystem), true);


TODO: We should switch this to the new resolveAndLoadEnvironment method. It'll do dependency resolution so we don't have to manually keep these lists in sync with every change.

keturn · 2021-04-04T18:02:34Z

Build #25 (20546f1) had 2 failing tests: http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/PR-4593/25/

it has now regressed to 43 failures.

and PlayerConfig-to-AutoConfig was inculded in build 25, so that wasn't the problem.

pollend · 2021-04-04T21:38:50Z

I decided to move the discord package since trying to register it to the environment is very difficult and harder to maintain if we introduce more subsystems. @keturn

keturn · 2021-04-10T03:06:30Z

I went back a few steps to see if I could reproduce the troubles you were having with DiscordRPC. and indeed I did! so I'm not sure how I had things working as well as I remember; maybe they worked until I merged in the AutoConfig changes from develop, and I didn't re-check after that? That's my best guess.

I also discovered that we seem to have the modules using the JsonSerializer for Reflections but engine and subsystems are still on XML, which is sure to be a source of confusion.

keturn · 2021-04-10T03:40:11Z

I plan to experiment a little to see if we can work things without sacrificing the engine/subsystem separation.

The plan is something like:

Make sure engine's Reflections are actually being loaded.
try re-instituting enrichReflectionsWithSubsystems
and probably switch all the Reflections caches to the JSON serializer for consistency, even if it's not strictly required by the above

That branch can be found at #4622

keturn · 2021-04-14T01:38:45Z

@pollend Which thing was it that called for the engine-test/…/module.txt file to move?

pollend · 2021-04-14T03:57:35Z

uuuh, I don't remember why

@pollend Which thing was it that called for the engine-test/…/module.txt file to move?

jdrueckert · 2021-04-16T20:18:29Z

[GESTALTv7] migrate module & asset

keturn · 2021-05-02T00:52:34Z

superseded by #4622

pollend commented Mar 21, 2021

View reviewed changes

keturn reviewed Mar 22, 2021

View reviewed changes

Comment thread engine-tests/src/test/java/org/terasology/documentation/BindingScraper.java Outdated

keturn reviewed Mar 22, 2021

View reviewed changes

Comment thread engine/build.gradle Outdated

keturn mentioned this pull request Mar 24, 2021

gestaltv7 compability for NUI 2.0 MovingBlocks/TeraNUI#27

Closed

keturn added this to the v4.4.0 milestone Mar 24, 2021

pollend force-pushed the feature/migrate-gestalt-v7 branch from c83bac4 to 6854c95 Compare March 27, 2021 03:21

feature: migrate to gestaltv7

e55b7f0

pollend force-pushed the feature/migrate-gestalt-v7 branch from 6854c95 to e55b7f0 Compare March 28, 2021 01:19

bugfix: correct copy strategy

f9ce52f

pollend mentioned this pull request Mar 28, 2021

chore: remove usage of RegisterCopyStrategy MovingBlocks/TeraNUI#50

Open

pollend added 2 commits March 28, 2021 08:40

chore: remove copy strategy

46c29ee

fix formatting

5504449

keturn added 3 commits April 1, 2021 12:52

refactor(handleSwitchToGameEnvironment): these are all one copy strategy

9afe704

build: no dependency on gestalt-entity-system yet

850ee40

Merge branch 'nui-gestalt-separation' into feature/migrate-gestalt-v7

911110a

chore(ComponentSystem): debugging logs

d9af0ba

pollend and others added 3 commits April 4, 2021 09:51

feature: migrate gestalt v7 discord subsystem (#4613)

c69a87f

* feature: add subsystem to modulemanager * bugfix: correct crash with ModuleManager

Merge remote-tracking branch 'origin/develop' into feature/migrate-ge…

d8de0d3

…stalt-v7

bugfix: correct newGameScreen

3c5ff6e

keturn reviewed Apr 4, 2021

View reviewed changes

move discord to engine namespace

873dec8

Merge branch 'develop' into feature/migrate-gestalt-v7

ccec83d

keturn mentioned this pull request Apr 10, 2021

chore: migrate to gestalt v7—fork 🅰, keep subsystems separate #4622

Merged

5 tasks

		private Module loadEngineModule(List<Class<?>> classesOnClasspathsToAddToEngine) {
		// FIXME: is `classes…toAddToEngine` gone? Did we ever use it in the first place?

		// private void enrichReflectionsWithSubsystems(Module engineModule) {
		// Serializer serializer = new XmlSerializer();

Conversation

pollend commented Mar 21, 2021 • edited by keturn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Issues

Module Changes

Uh oh!

pollend commented Mar 21, 2021

Uh oh!

pollend Mar 21, 2021

Choose a reason for hiding this comment

Uh oh!

keturn commented Mar 21, 2021

Uh oh!

keturn commented Mar 22, 2021

Uh oh!

Uh oh!

Uh oh!

pollend commented Mar 22, 2021

Uh oh!

pollend commented Mar 22, 2021

Uh oh!

DarkWeird commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keturn commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkWeird commented Mar 22, 2021

Uh oh!

keturn commented Mar 24, 2021

Uh oh!

keturn commented Mar 29, 2021

Uh oh!

pollend commented Mar 29, 2021

Uh oh!

keturn commented Apr 2, 2021

Uh oh!

keturn commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keturn commented Apr 2, 2021

Uh oh!

keturn left a comment

Choose a reason for hiding this comment

Uh oh!

keturn Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

keturn Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

keturn Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

keturn Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

pollend Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

keturn Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

keturn commented Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pollend commented Apr 4, 2021

Uh oh!

keturn commented Apr 10, 2021

Uh oh!

keturn commented Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keturn commented Apr 14, 2021

Uh oh!

pollend commented Apr 14, 2021

Uh oh!

jdrueckert commented Apr 16, 2021

Uh oh!

pollend commented Mar 21, 2021 •

edited by keturn

Loading

DarkWeird commented Mar 22, 2021 •

edited

Loading

keturn commented Mar 22, 2021 •

edited

Loading

keturn commented Apr 2, 2021 •

edited

Loading

keturn commented Apr 4, 2021 •

edited

Loading

keturn commented Apr 10, 2021 •

edited

Loading

keturn commented May 2, 2021 •

edited

Loading