Terasology engine #5192

soloturn · 2023-12-06T04:55:46Z

inttelij, checkstyle, pmd warnings removed:

case of constants
line length
make fields final
remove unnecessary toString()
ordering of declarations
unused fields
simplify conditions

soloturn · 2023-12-07T22:20:28Z

is there a way to retrigger the jenkins build, apart from force pushing?

BenjaminAmos · 2023-12-07T23:51:05Z

Under the checks tab on GitHub you should be able to re-run the tests if they fail. That appears to trigger a Jenkins re-run, as of recently (give https://github.com/MovingBlocks/Terasology/pull/5188/checks?check_run_id=19397149388 a try, for example). Some of the core contributors can force retrigger a build from the Jenkins UI but this relies on being added to a manual whitelist in the Logistics repository right now. If you're on that list then you can sign-in to Jenkins with your GitHub account to do that.

soloturn · 2023-12-08T08:11:47Z

ok now checks are green. @BenjaminAmos @jdrueckert you mind merging?

jdrueckert · 2023-12-12T10:59:04Z

Under the checks tab on GitHub you should be able to re-run the tests if they fail. That appears to trigger a Jenkins re-run, as of recently (give https://github.com/MovingBlocks/Terasology/pull/5188/checks?check_run_id=19397149388 a try, for example). Some of the core contributors can force retrigger a build from the Jenkins UI but this relies on being added to a manual whitelist in the Logistics repository right now. If you're on that list then you can sign-in to Jenkins with your GitHub account to do that.

Alternatively, instead of force pushing, you can create an empty commit using the allow-empty parameter for git commit.

engine/src/main/java/org/terasology/engine/world/time/WorldTimeImpl.java

BenjaminAmos · 2023-12-16T21:26:07Z

engine/src/main/java/org/terasology/engine/audio/formats/OggReader.java

-    /**
-     * The mono 16 bit format
-     */
-    private static final int FORMAT_MONO16 = 1;
-
-    /**
-     * The stereo 16 bit format
-     */
-    private static final int FORMAT_STEREO16 = 2;
-


Why did you remove these constants? They still appear to be relevant. It may be unused now but it could have been useful in the future.

BenjaminAmos · 2023-12-16T21:27:37Z

engine/src/main/java/org/terasology/engine/audio/formats/OggReader.java

-                    convbuffer[ptr + 0] = (byte) (bigEndian ? val >>> 8 : val);
-                    convbuffer[ptr + 1] = (byte) (bigEndian ? val : val >>> 8);
+                    CONVBUFFER[ptr] = (byte) (bigEndian ? val >>> 8 : val);
+                    CONVBUFFER[ptr + 1] = (byte) (bigEndian ? val : val >>> 8);


In this case, the ptr + 0 may have been for clarity. It helps to keep the lines in alignment.

BenjaminAmos · 2023-12-16T21:29:41Z

engine/src/main/java/org/terasology/engine/logic/chat/ChatSystem.java

-    private String join(String[] words, String sep) {
-        return Arrays.stream(words).collect(joining(sep));
+    private String join(String[] words) {
+        return String.join(" ", words);
    }


If you're going to change the purpose of the method, you might as well re-name it too. joinWithWhitespace seems more appropriate now than just a generic join.

BenjaminAmos · 2023-12-16T21:33:15Z

engine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.java

-                if (hibernationSettings.isPresent()) {
-                    hibernationSettings.get().setHibernationAllowed(false);
-                }
+                hibernationSettings.ifPresent(hibernationManager -> hibernationManager.setHibernationAllowed(false));


Using functional methods, whilst more concise, have in the past caused performance issues. Streams and List#forEach were particularly poor in comparison to a for-each loop, Since this is in network code, I am slightly concerned about the potential impact here.

BenjaminAmos · 2023-12-16T21:34:18Z

engine/src/main/java/org/terasology/engine/network/internal/NetworkSystemImpl.java

@@ -448,7 +444,7 @@ public void registerNetworkEntity(EntityRef entity) {
            netComponent.setNetworkId(nextNetId++);
            entity.saveComponent(netComponent);
            netIdToEntityId.put(netComponent.getNetworkId(), entity.getId());
-            switch (netComponent.replicateMode) {
+            switch (java.util.Objects.requireNonNull(netComponent.replicateMode)) {


Is there a reason why this is fully-qualified? Usually you'd add an import for this, unless the name conflicts with another imported class.

BenjaminAmos · 2023-12-16T21:41:09Z

engine/src/main/java/org/terasology/engine/utilities/OperatingSystemMemory.java

-        /** unused since Linux 2.6 */
-        LIB(4),
        /** data + stack */
-        DATA(5),
-        /** unused since Linux 2.6 */
-        DT(6);
+        DATA(5);


I know that this says unused but as an enum the order of members matters. Removing enum members will re-order the existing ones, which might cause them to have the wrong ordinal values. It's quite normal for structs mapping low-level OS fields to contain "unused" or obsolete values. It's to maintain ABI compatibility.

engine/src/main/java/org/terasology/engine/world/chunks/blockdata/ExtraBlockDataManager.java

engine/src/main/java/org/terasology/engine/world/time/WorldTimeImpl.java

soloturn · 2023-12-20T17:42:55Z

apart from one, reverted as mentioned in your review @BenjaminAmos .

engine/src/main/java/org/terasology/engine/audio/formats/OggReader.java

soloturn · 2023-12-22T08:45:22Z

@BenjaminAmos i did your requested change, but still it shows "change requested". it means you need to close this?

BenjaminAmos · 2023-12-23T21:31:02Z

engine/src/main/java/org/terasology/engine/logic/console/ConsoleSystem.java

-                if (!nuiManager.isOpen("engine:console") && message.getType() != CoreMessageType.CHAT
-                        && message.getType() != CoreMessageType.NOTIFICATION || !nuiManager.isOpen("engine:chat")) {
-                    // make sure the message isn't already shown in the chat overlay
+                // make sure the message isn't already shown in the chat overlay
+                if (!nuiManager.isOpen("engine:console")
+                        && (message.getType() != CoreMessageType.CHAT
+                        && message.getType() != CoreMessageType.NOTIFICATION
+                        || !nuiManager.isOpen("engine:chat"))) {


This is a difficult condition to parse. I think you have rearranged it correctly but if a bug appears in this area then we might have to re-visit it.

checkstyle and pmd warnings handled. drop the whole class as it seems not to be used?

fix checkstyle and pmd warnings.

fix checkstyle, pmd warnings.

fix checkstyle, pmd warnings, like "unchecked assignment" of TeraArray.Factory<? extends TeraArray>[] slotFactories;

fix checkstyle, pmd warnings.

soloturn force-pushed the terasology-engine branch from e6f7849 to 0ebf391 Compare December 7, 2023 06:21

soloturn requested review from jdrueckert and BenjaminAmos December 7, 2023 22:18

soloturn force-pushed the terasology-engine branch from 0ebf391 to ad475b3 Compare December 7, 2023 22:20

BenjaminAmos reviewed Dec 16, 2023

View reviewed changes

engine/src/main/java/org/terasology/engine/world/time/WorldTimeImpl.java Outdated Show resolved Hide resolved

BenjaminAmos reviewed Dec 16, 2023

View reviewed changes

BenjaminAmos requested changes Dec 20, 2023

View reviewed changes

engine/src/main/java/org/terasology/engine/audio/formats/OggReader.java Outdated Show resolved Hide resolved

soloturn force-pushed the terasology-engine branch from 17a7d00 to e0ef9e0 Compare December 20, 2023 22:59

soloturn requested a review from BenjaminAmos December 20, 2023 23:07

BenjaminAmos approved these changes Dec 23, 2023

View reviewed changes

soloturn added 12 commits December 24, 2023 23:29

performance benchmark qa. drop class?

d3a1ae5

checkstyle and pmd warnings handled. drop the whole class as it seems not to be used?

engine audio qa.

90d5684

fix checkstyle and pmd warnings.

engine logic qa.

307673c

fix checkstyle, pmd warnings.

engine network qa.

c1f7602

fix checkstyle, pmd warnings.

engine world qa.

612e610

fix checkstyle, pmd warnings, like "unchecked assignment" of TeraArray.Factory<? extends TeraArray>[] slotFactories;

engine rendering qa.

9d25613

fix checkstyle, pmd warnings.

engine telemetry qa.

5931d2d

fix checkstyle, pmd warnings.

engine utilities qa.

38655dd

fix checkstyle, pmd warnings.

engine world qa, unused v0 in graph creation.

3aa3711

unused constants removed from operationgsystemmemory.

07c6a70

qa engine, BenjaminAmos feedback, valid warning here.

4ca4e5b

qa engine, BenjaminAmos feedback.

16f402b

soloturn force-pushed the terasology-engine branch from e0ef9e0 to 16f402b Compare December 24, 2023 22:29

soloturn merged commit 14937d2 into MovingBlocks:develop Dec 24, 2023
9 checks passed

soloturn deleted the terasology-engine branch December 24, 2023 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Terasology engine #5192

Terasology engine #5192

soloturn commented Dec 6, 2023 •

edited by jdrueckert

Loading

soloturn commented Dec 7, 2023

BenjaminAmos commented Dec 7, 2023

soloturn commented Dec 8, 2023

jdrueckert commented Dec 12, 2023

BenjaminAmos Dec 16, 2023

BenjaminAmos Dec 16, 2023

BenjaminAmos Dec 16, 2023

BenjaminAmos Dec 16, 2023

BenjaminAmos Dec 16, 2023

BenjaminAmos Dec 16, 2023

soloturn commented Dec 20, 2023

soloturn commented Dec 22, 2023

BenjaminAmos Dec 23, 2023

Terasology engine #5192

Terasology engine #5192

Conversation

soloturn commented Dec 6, 2023 • edited by jdrueckert Loading

soloturn commented Dec 7, 2023

BenjaminAmos commented Dec 7, 2023

soloturn commented Dec 8, 2023

jdrueckert commented Dec 12, 2023

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

BenjaminAmos Dec 16, 2023

Choose a reason for hiding this comment

soloturn commented Dec 20, 2023

soloturn commented Dec 22, 2023

BenjaminAmos Dec 23, 2023

Choose a reason for hiding this comment

soloturn commented Dec 6, 2023 •

edited by jdrueckert

Loading