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.

Sign up for GitHub

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

Jump to bottom

Open up the DebugOverlay to add/remove MetricsModes #2316

Merged
merged 8 commits into from
May 26, 2016
Merged

Open up the DebugOverlay to add/remove MetricsModes #2316

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
751b17d
Add ListOrderedSet, Edit DebugOverlay and MetricsMode, register and u…
tdgunes May 11, 2016
f22f5bc
Remove ListOrderedSet, Add DebugRenderingSystem
tdgunes May 14, 2016
0710a66
Rename DebugRenderingSystem to DebugMetricsSystem, add JavaDocs, guar…
tdgunes May 19, 2016
e7c853b
Add CircularToggleSet (a new data structure), DebugMetricsSystemTest,…
tdgunes May 21, 2016
faa2f24
Fix corner case, removing defaultMode, add its test
tdgunes May 21, 2016
92dfda5
Remove collection.CircularToggleSet, add a primitive version to Debug…
tdgunes May 22, 2016
e42d6b9
Edit DebugMetricsSystem, remove CircularToggleSet
tdgunes May 23, 2016
fa3ef13
Refactor wild card import
tdgunes May 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions engine/src/main/java/org/terasology/rendering/nui/layers/ingame/metrics/DebugOverlay.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014 MovingBlocks
* Copyright 2016 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,7 +15,6 @@
*/
package org.terasology.rendering.nui.layers.ingame.metrics;

import com.google.common.collect.Lists;
import org.terasology.config.Config;
import org.terasology.engine.GameEngine;
import org.terasology.engine.Time;
Expand All @@ -37,7 +36,6 @@
import org.terasology.world.biomes.Biome;
import org.terasology.world.biomes.BiomeManager;

import java.util.List;
import java.util.Locale;

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add class-level javadoc. In a short paragraph somebody looking at this class should immediately understand:

  • what is the purpose of the DebugOverlay
  • what fixed information it provides and in what format
  • how the dynamic information is generated and handled, i.e. what methods manipulate the dynamic portion of the overlay

I agree this information should have been there in the first place, written by the original author of the class.

Expand Down Expand Up @@ -65,12 +63,10 @@ public class DebugOverlay extends CoreScreenLayer {
@In
private WorldProvider worldProvider;

private List<MetricsMode> metricsModes = Lists.newArrayList(new NullMetricsMode(), new RunningMeansMode(), new SpikesMode(),
new AllocationsMode(), new RunningThreadsMode(), new WorldRendererMode(), new NetworkStatsMode(),
new RenderingExecTimeMeansMode("Rendering - Execution Time: Running Means - Sorted Alphabetically"));
private int currentMode;
private UILabel metricsLabel;
@In
private DebugRenderingSystem debugRendering;

private UILabel metricsLabel;

@In
private StorageManager storageManager;
Expand Down Expand Up @@ -167,8 +163,10 @@ public Boolean get() {

@Override
public void update(float delta) {
if (metricsLabel != null) {
metricsLabel.setText(metricsModes.get(currentMode).getMetrics());
if (debugRendering.isMetricModesAvailable()) {
metricsLabel.setText(debugRendering.getCurrentMode().getMetrics());
} else {
metricsLabel.setText("No metric modes are available!");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of explicitly checking whether the list is empty of not, you could add a default dummy mode and guarantee that getCurrentMode is never null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, isn't that what NullMetricsMode is for?

}
}

Expand All @@ -183,10 +181,11 @@ public boolean isEscapeToCloseAllowed() {
}

Copy link
Contributor

@emanuele3d emanuele3d May 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
Moves forward through the MetricsMode instances and displays the content of the next available one.
*/

public void toggleMetricsMode() {
currentMode = (currentMode + 1) % metricsModes.size();
while (!metricsModes.get(currentMode).isAvailable()) {
currentMode = (currentMode + 1) % metricsModes.size();
if (debugRendering.isMetricModesAvailable()) {
debugRendering.toggle();
PerformanceMonitor.setEnabled(debugRendering.getCurrentMode().isPerformanceManagerMode());
}
PerformanceMonitor.setEnabled(metricsModes.get(currentMode).isPerformanceManagerMode());
}


}
78 changes: 78 additions & 0 deletions ...rc/main/java/org/terasology/rendering/nui/layers/ingame/metrics/DebugRenderingSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/*
* Copyright 2016 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.terasology.rendering.nui.layers.ingame.metrics;


import com.google.common.collect.Iterators;
import com.google.common.collect.Sets;
import org.terasology.entitySystem.systems.BaseComponentSystem;
import org.terasology.entitySystem.systems.RegisterSystem;
import org.terasology.registry.Share;

import java.util.Iterator;
import java.util.LinkedHashSet;

@RegisterSystem
@Share(DebugRenderingSystem.class)
public class DebugRenderingSystem extends BaseComponentSystem {

private LinkedHashSet<MetricsMode> metricsModes;
private Iterator<MetricsMode> modeIterator;
private MetricsMode currentMode;

@Override
public void initialise() {
metricsModes = Sets.newLinkedHashSet();
modeIterator = Iterators.cycle(metricsModes);
currentMode = register(new NullMetricsMode());
register(new RunningMeansMode());
register(new SpikesMode());
register(new AllocationsMode());
register(new RunningThreadsMode());
register(new WorldRendererMode());
register(new NetworkStatsMode());
register(new RenderingExecTimeMeansMode("Rendering - Execution Time: Running Means - Sorted Alphabetically"));
}

public MetricsMode register(MetricsMode mode) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a hunch that something wasn't quite right with this class but I couldn't quite point the finger to it. I now realize that issue #1773 recommends to add register() and unregister() methods to the DebugOverlay class. That's because DebugOverlay is not intended to be rendering-specific. Systems or even modules might wish to register their own MetricsMode with it, completely unrelated to rendering.

This is also in line with the NetworkStatsMode() above not being quite rendering-related. It is true that at this stage the DebugOverlay is mostly used for rendering but it's not intended to be exclusive for it.

So, I recommend to remove this class and move its functionality into DebugOverlay. I.e. I like the switch to a LinkedHashSet and the use of register() to register the default Metrics.

Eventually we should move the registration of the default rendering metrics into the WorldRenderer initialization.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this class has a right to exist. In order to register a MetricsMode, the system can be injected from any module. I think that the DebugOverlay should only display info. on user interface.

A similar approach can be found for the console. There's ConsoleScreen for rendering and ConsoleSystem that contains the actual content.

If the default metrics go into the world rendering init. code, we need to make sure that this is called afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds like a separation of presentation/content issue. You'd like this class take care of the content (avilable through the MetricModes instances) and then DebugOverlay takes care of presenting the data on screen.

Ok, I understand the approach and I'm fine with it. I would then recommend to rename the DebugRenderingSystem class to DebugMetricsSystem, so that it is not rendering-specific and is clearly associated with the concept of Debug Metrics.

Regarding the default metrics, I'd like to be clear that only the rendering-related metrics would be instantiated and registered during the rendering engine initialization. The DebugMetricsSystem by then should be up and running already. And on shutdown the rendering engine should make sure to unregister all its metrics. But this is out of the scope of this PR. I'm happy to leave those things as they are for the time being.

Finally, as this class is bound to stay, it will need some javadocs then. Class-level AND non-overriding public methods pleaaaaaaase!!! =)

metricsModes.add(mode);
return mode;
}

public MetricsMode getCurrentMode() {
return currentMode;
}

public void toggle() {
do {
currentMode = modeIterator.next();
} while (!getCurrentMode().isAvailable());
}

public boolean isMetricModesAvailable() {
return !metricsModes.isEmpty();
}

public void unregister(MetricsMode mode) {
metricsModes.remove(mode);
}

public void unregisterAll() {
metricsModes.clear();
}


}
2 changes: 1 addition & 1 deletion engine/src/main/java/org/terasology/rendering/nui/layers/ingame/metrics/MetricsMode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2014 MovingBlocks
* Copyright 2016 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down