Create a new item that contains only the necessary visual components … #3137

portokaliu · 2017-10-31T11:18:19Z

This should be a fix to #3126

I've reworked a bit of the system to use as few new entities as really needed. No more items will be lost and no objects floating in the world upon loading.

The downsides are:

There's always gonna be a null warning when switching to a block type item because of the way it's constructed from the prefab.
Any new visual stuff that we might want later to be visible first person will have to be manually implemented here (via adding the component) (This can be avoided by making the visual related components inherit a "Visual Component", but currently the system doesn't store components properly for such items (usually blocks), so that implementation will be postponed (unless there's time to look into that as well)

…that can be destroyed when we change items. ( workaround to copying entities not really working as intended )

GooeyHub · 2017-10-31T11:25:48Z

Hooray Jenkins reported success with all tests good!

aaron-harris · 2017-10-31T15:18:32Z

engine/src/main/java/org/terasology/logic/players/FirstPersonClientSystem.java

@@ -103,7 +100,7 @@ public void ensureHeldItemIsMountedOnLoad(OnChangedComponent event, EntityRef en
            CharacterHeldItemComponent characterHeldItemComponent = localPlayer.getCharacterEntity().getComponent(CharacterHeldItemComponent.class);
            if (characterHeldItemComponent != null) {
                // special case of sending in null so that the initial load works
-                linkHeldItemLocationForLocalPlayer(characterHeldItemComponent.selectedItem, null);
+                linkHeldItemLocationForLocalPlayer(characterHeldItemComponent.selectedItem);


The comment immediately above this now appears to be out of date.

syntaxi · 2017-11-05T02:46:07Z

@bbarker, this is the fix in question.

bbarker · 2017-11-05T13:34:17Z

@jellysnake thanks!

I was able to cherry pick and merge this on top of 1.5.0 and it seems to run. I'd like to drop it in as a replacement to my existing stable/Launcher install of Tersalogy if possible for further testing (and fun). As I'm on Linux this looks to be under ~/.terasology. However, I'm not sure exactly which jars should be replaced. In my build directory I have just a few jars:

$ find ./ -name '*.jar'
./engine/build/libs/engine-1.4.1-SNAPSHOT.jar
./engine/libs/jopenvr.jar
./engine-tests/build/libs/engine-tests-1.4.1-SNAPSHOT.jar
./facades/PC/build/libs/PC-1.4.1-SNAPSHOT.jar
./facades/TeraEd/build/libs/TeraEd-1.4.1-SNAPSHOT.jar
./gradle/wrapper/gradle-wrapper.jar
./modules/BuilderSampleGameplay/build/libs/BuilderSampleGameplay-1.4.1-SNAPSHOT.jar
./modules/Core/build/libs/Core-1.4.1-SNAPSHOT.jar
./modules/CoreSampleGameplay/build/libs/CoreSampleGameplay-1.4.1-SNAPSHOT.jar

They appear to be mislabeled in their filename; this was checked out from 1.5.0, but anyway, I suspect I should just copy ./engine/build/libs/engine-1.4.1-SNAPSHOT.jar to ~/.terasology/RELEASE/TerasologyStable/72/libs/engine-1.5.0.jar in this case? Maybe also ./facades/TeraEd/build/libs/TeraEd-1.4.1-SNAPSHOT.jar to ~/.terasology/RELEASE/TerasologyStable/72/libs/Terasology.jar?

portokaliu · 2017-11-06T08:21:33Z

Fixed it according to the mentioned issues. Now there shouldn't be any null warnings present and any module can add visual stuff to the item as long as the component implements VisualComponent

GooeyHub · 2017-11-06T08:28:21Z

Hooray Jenkins reported success with all tests good!

syntaxi · 2017-11-06T11:43:46Z

@bbarker I'm actually not sure. @Cervator might know. Not sure who else. I presume that copying those should work but I'm not fully sure.

bbarker · 2017-11-06T12:51:19Z

@jellysnake @Cervator Cool, not a big deal but was thinking that might be the way to go about testing (and then making) a 1.5.1 release - correct me if I'm wrong.

Cervator · 2017-11-06T22:51:34Z

So we haven't really needed true hot fixes to past releases before, but maybe this would be an interesting experiment :-)

I'm not sure what happened with those v1.4.1-SNAPSHOT jars - those would have come from a develop build before Alpha 8 / v1.5.0 was released. So some point before e5d5b73

The game Launcher maintains two dirs: installs and data. Each version of the game it maintains gets its own, or at least its own install dir, I forget.

The jars in the v1.5.0 dir there should be updated to make the game executed by the launcher use a hot fix like that. The install + data dir defaults are probably both under your home dir.

To do a proper hot fix we'd branch off master or the v1.5.0 tag (same thing at the moment) to add a single commit to fix this on top. Interestingly that would produce v1.5.1 - which is also the current next release that would come out of develop if it were released (realistically it would become v1.6.0 as we've added more than just fixes by now, but that determination is usually not made until we release)

I'd guess we would make such a branch then build, test, and release v1.5.1 (again pushing to master and making a tag), then immediately after merge it to develop and bump it to v1.5.2-SNAPSHOT or even v1.6.0-SNAPSHOT as we know by now the next release will have new features and not just patch fixes. We just haven't needed to keep the version number updated between releases before.

Adhering to SemVer is fun!

As to the actual bug at hand yay simplifying code. Although is introducing VisualComponent as a child interface of Component that other components sometimes implement a risk to the trait-based architecture? Since the component hierarchy is meant to be very flat. I don't know if it makes sense to have some special cases like this. It might? I'd be curious to hear from others that are more architecture minded :-)

For the testing I can say things are better (again) - but unless my workspace is dusty I was still able to provoke it, just back down at 0,0,0 :-)

After one reload, single player:

And a second:

I could also replicate it for the host in a server-client + client multiplayer session, but not for the client, and I expect headless would be entirely free from the issue. So we're back to about 99% fixed + merge-worthy IMHO. Unsure if it is worth doing a full round of hotfix release experimenting, although we rarely get such opportunities so maybe :-)

TL;DR: Nearly fixed, happy to merge it, but wouldn't mind hearing a bit more feedback first

portokaliu · 2017-11-07T09:09:41Z

Right, about that. Was too focused on the "not appear where you load" and not breaking inventory and stuff that I managed to forget about the core issue that brought this all up.

If making a child interface isn't a good idea, we can always go back to manually doing everything, or if anyone has a better idea I'm open to suggestions. Will also think about what other cases might there be for child interfaces and how it might impact code. Certainly found my fair share of anomalies.

Should have another update today to fix that, think I've found a reason for it existing but I'll investigate why the save system still wants to keep the item...

portokaliu · 2017-11-07T12:24:29Z

I've submitted a fix that bypasses the issue with FP items still being saved. Still need to look into why they're saved in the first place, but that may be in an unrelated issue

GooeyHub · 2017-11-07T12:29:27Z

Hooray Jenkins reported success with all tests good!

Cervator · 2017-11-14T01:55:59Z

Poked about some more and couldn't find any issues. Yay! Maybe this one is finally done for good :-)

Thanks @portokaliu and others!

Create a new item that contains only the necessary visual components …

4b36073

…that can be destroyed when we change items. ( workaround to copying entities not really working as intended )

aaron-harris reviewed Oct 31, 2017

View reviewed changes

Fixed first person items issues.

083ec8b

Fix item appearing at (0,0,0) for host

f59b1b5

Cervator added the Type: Bug Issues reporting and PRs fixing problems label Nov 14, 2017

Cervator added this to the Alpha 9 milestone Nov 14, 2017

Cervator merged commit f59b1b5 into MovingBlocks:develop Nov 14, 2017

Cervator added a commit that referenced this pull request Nov 14, 2017

Merge PR #3137 by @portokaliu - bug fix

7ed79cd

portokaliu deleted the FPItemFix branch November 1, 2018 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a new item that contains only the necessary visual components … #3137

Create a new item that contains only the necessary visual components … #3137

portokaliu commented Oct 31, 2017

GooeyHub commented Oct 31, 2017

aaron-harris Oct 31, 2017

syntaxi commented Nov 5, 2017

bbarker commented Nov 5, 2017 •

edited

Loading

portokaliu commented Nov 6, 2017

GooeyHub commented Nov 6, 2017

syntaxi commented Nov 6, 2017

bbarker commented Nov 6, 2017

Cervator commented Nov 6, 2017

portokaliu commented Nov 7, 2017

portokaliu commented Nov 7, 2017

GooeyHub commented Nov 7, 2017

Cervator commented Nov 14, 2017

Create a new item that contains only the necessary visual components … #3137

Create a new item that contains only the necessary visual components … #3137

Conversation

portokaliu commented Oct 31, 2017

GooeyHub commented Oct 31, 2017

aaron-harris Oct 31, 2017

Choose a reason for hiding this comment

syntaxi commented Nov 5, 2017

bbarker commented Nov 5, 2017 • edited Loading

portokaliu commented Nov 6, 2017

GooeyHub commented Nov 6, 2017

syntaxi commented Nov 6, 2017

bbarker commented Nov 6, 2017

Cervator commented Nov 6, 2017

portokaliu commented Nov 7, 2017

portokaliu commented Nov 7, 2017

GooeyHub commented Nov 7, 2017

Cervator commented Nov 14, 2017

bbarker commented Nov 5, 2017 •

edited

Loading