refactor! move engine code in to org.terasology.engine package

[keturn]

In the engine project:

• org.terasology.engine.* → org.terasology.engine.core.*
• org.terasology.* → org.terasology.engine.*

See this comment for a migration file: #4560 (comment)

In engine-test:

• it is a confused place, but the tests which need to be in the same package as their code-under-test have been updated to match.

Subsystems remain the same:

• DiscordRPC: org.terasology.engine.subsystem.discordrpc has been moved in develop
• TypeHandlerLibrary: org.terasology.persistence & org.terasology.reflection.reflect

Facades remain the same:

• PC: org.terasology.engine
• TeraEd: org.terasology.editor

Since the engine package seems to be a blocker for starting gestalt-v7,
and nobody wants to move the packages for fear file moves causing git conflicts with other work in progress,
here is a wacky and whimsical alternative:

Leave the files in place, but use some config overrides to make them think they are in a package named something else.

I don't recommend leaving it configured this way for very long because someone or something WILL get terribly confused, but maybe it helps get the ball rolling?

warning: I failed to learn all the lessons from #4144 and forgot to turn off the copyright-updater before doing mass file operations.

[keturn]

I wonder if someone has published a diff filter that makes it easy to say "hide all chunks that change only imports or comments."

[keturn]

Recent versions of git (2.30) have an option to --ignore-matching-lines

I find this almost does what I want:

git diff \
    --ignore-matching-lines='^(@API )?(package|import)' \
    --ignore-matching-lines='^/[/*]' \
    --ignore-matching-lines='^ \*' \
  origin/develop...origin/chore/move-in-place

but some things are still slipping through. I think because they have a blank line in them? but my attempts to filter out blank lines with something like ^$ seem to hide everything.

Update: removing context lines with -U0 --patience removes the context, but helps some, I think because hunks that contained a blank line get split in to separate hunks around the blank line.

[keturn]

@DarkWeird Is this the right package organization?

Do we need a stricter policy on what packages things like facade and subsystems use?

This splits the existing org.terasology.persistence package in an odd way, because I've left the TypeHandlerLibrary sources under that package, but there are sources in engine that are now org.terasology.engine.persistence. Is that okay for now, or should that be different?

(I did take a quick look to see if any of your current open PRs address that kind of thing for TypeHandlerLibrary, but I didn't find any.)

[DarkWeird]

I think that org.terasology.sibsystem better.
Then adds to engine's whitelist "trusted sources"

[keturn]

There is one test failure in this branch: http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/view/change-requests/job/PR-4560/4/testReport/org.terasology.engine.config.flexible/AutoConfigSerializerTest/testSerializeNonDefault__/

an AutoConfigSerializerTest that says

expected: <{"integerListSetting":[1,2,3],"Human Readable Name":"xyz"}>
 but was: <{"Human Readable Name":"xyz","integerListSetting":[1,2,3]}>

I guess for a serializer, the ordering should be stable. But I'm confused if it's a bug that's really specific to this branch.

[keturn]

If I had all of modulespace checked out in to one IntelliJ project when I did the package-move refactorings, I suppose IntelliJ would have updated all the module code for me, if it didn't explode from the size of it.

But I didn't, so now I'm trying to figure out how to apply those changes to the modules after the fact.

I was about to try some regular expression search-and-replace when I came across IntelliJ's migration feature.

If you grab the xml file from this gist: https://gist.github.com/keturn/9a6b598c11ba7f9c3b710c8fe1e3a6c5
drop it in the migration folder of your local IntelliJ configuration directory (e.g. ~/.config/JetBrains/IntelliJIdea2020.3/migration/), and restart,
then you can go to the Refactor/Migrate and choose this "Terasology Engine Package" migration map.

Caveats:

• I haven't figured out how to apply it to only one scope, so it'll try to do the entire multi-project, even though this branch should already be migrated for the stuff in the main repository.
  • it does do a preview phase, and you can mark things as "excluded" there, that might help
• There might be still some stuff left over to fix by hand. Some of the packages in engine's src/main/java/org/terasology/ overlap with those in TeraNUI or the typehandler library or TeraMath or other such things, so the migration map isn't perfect.

[keturn]

but most worryingly, I can't get the result to build. https://scans.gradle.com/s/3w3pazserxguc

Lots of "could not find class" stuff that looks like it should be available. Not sure if I have skewed versions somehow, or if my build is cursed by the "moving packages without moving files" trick.

[keturn]

That scan says that only DynamicCities and SimpleFarming actually failed, so if I remove those from my workspace, it does pass https://scans.gradle.com/s/vbjifnzxkhooa

but there is a lot of noise from reflections in the log about not being able to find classes. probably during the cache-reflections step while building module jars. I haven't checked to see if that's because it hates the file layout in this branch, or if it's something the recent upgrade to 0.9.12 caused.

[keturn]

okay, have made some progress. I have all of CoreSampleGameplay building and I can at least attempt to start a single-player game without getting any module resolution failures.

I guess that means I should check stuff in to a branch on all those modules?

[keturn]

Ah, how could I have forgotten about Master of Oreon!

I checked out Omega and gave gradle more RAM and I think that should be all of them now.

If I haven't created a chore/move-in-place branch in a module, then either it didn't get checked out when I did groovyw module init omega, or the migration didn't find any changes to make in it.

[keturn]

I'm still haven't decided what to do about that one test failure. I see a PR #4235 with "autoconfig" in the title, so I've kinda been waiting to see if it gets merged before this one and it deals with the problem somehow.

[jdrueckert]

If I haven't created a chore/move-in-place branch in a module, then either it didn't get checked out when I did groovyw module init omega, or the migration didn't find any changes to make in it.

Just double-checked and it seems like all modules that don't have a chore/move-in-place branch are pure asset modules (or in addition only adding prefab deltas/overrides) 👍

[jdrueckert]

I won't look at each file and into all module PRs, but I checked out omega with all the chore/move-in-place branches, it compiled and JS, MR and LaS started and looked as expected.

[keturn]

…working on the merge conflicts…

[keturn]

Merged the autoconfig PR, but the same test still fails here. http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/PR-4560/10/testReport/junit/org.terasology.engine.config.flexible/AutoConfigSerializerTest/testSerializeNonDefault__/

Thing is, if the output order isn't stable because HashMap ordering isn't stable in general, I'd expect it to be a flaky test. But it hasn't been failing in develop, and it seems to be failing every build here.

1. Is the ordering a significant quality of the test? If not, we can use some other comparison technique for the assertion that is not sensitive to order.
2. why is the order different in this branch? Is it somehow sensitive to the fully qualified class names of the objects involved somehow?

my hunch is that we've somehow uncovered a bug that shows the output is not stable, and it doesn't really have anything to do with this branch. In that case, should we disable the test?

or is a fix small enough to include in this branch?

I'd think it should either be insertion-order (that's how it's written now), or some locale-independent sort on the key.

[keturn]

getSettingFields returns a Set from Reflections, so that ordering is undefined:

Terasology/engine/src/main/java/org/terasology/config/flexible/AutoConfig.java

Lines 24 to 25 in b00f1b9

	static Set<Field> getSettingFieldsIn(Class<? extends AutoConfig> configType) {
	return ReflectionUtils.getFields(

if we want to sort the output, this looks like the place to do it

Terasology/engine/src/main/java/org/terasology/config/flexible/AutoConfigSerializer.java

Lines 72 to 76 in b00f1b9

	public PersistedData serialize(C config, PersistedDataSerializer serializer) {
	// TODO: Serialize config name and description?
	Map<String, PersistedData> persistedSettings = Maps.newLinkedHashMap();
	for (Map.Entry<Field, TypeHandler<?>> fieldHandlerEntry : settingFieldHandlers.entrySet()) {

[keturn]

This is what I did about the AutoConfigSerializer test: dd09b91