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.

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

Open up the DebugOverlay to add/remove MetricsModes #2316

Merged
merged 8 commits into from May 26, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Member

tdgunes commented May 11, 2016

Contains

A partial fix for #1773, can be thought as a start too. As also mentioned in #1773, current usage of MetricModes in DebugOverlay does not allow runtime modifications and possibility of implementing data-processing objects which could be reused in other modes.

At the moment, PR adds new methods for DebugOverlay class:

public void register(MetricsMode mode);
public void unregister(MetricsMode mode); 

Moves default metrics to abstract MetricsMode class:

static void setDefaultMetrics(DebugOverlay debugOverlay) {
        debugOverlay.register(new RunningMeansMode());
        ...
        debugOverlay.register(new NetworkStatsMode());
}

In order to store MetricsMode objects in DebugOverlay, PR includes ListOrderedSet as a new collection which acts like a list and a unique set. Therefore, items are ordered and unique at the same time. It's based on Java's HashMap and ArrayList implementations. Name and some of the implementation aspects are inspired from Apache's ListOrderedSet. It's important to point out that this collection might be not the best choice in this scenario. It would be great to have some feedbacks especially on this.

How to test

  • For testing the collection, PR includes ListOrderedSetTest.
  • For checking whether PR broke current functionality of DebugOverlay, during in game, press F3 and then toggle metrics by F4.

Outstanding before merging

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 11, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 11, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 12, 2016

Contributor

@tdgunes, thank you for looking into this. I only had the chance to have a brief look into this. I'll go more in-depth over the next 48 hours.

Something that is certainly missing is a healthy dose of javadocs. They are not necessary for the test class and the ListOrderedSet just needs a class-level javadocs, the methods are self-explanatory.

However, as you probably had to look in-depth into what DebugOverlay and MetricsMode do, I'd encourage you to add class-level and method-level javadocs to them.

One thing I'm not convinced about is your somewhat special handling of the "DefaultMetrics". It seems to make things more complicated this way. I'd be happy for their instantiation to occur in the initialize() function, but I don't quite see the advantage in wrapping them in MetricModes.setDefaultMetrics(). I do like the use of the register() method to do it though.

By the way could you have used a LinkedHashSet instead of a custom class?

Contributor

emanuele3d commented May 12, 2016

@tdgunes, thank you for looking into this. I only had the chance to have a brief look into this. I'll go more in-depth over the next 48 hours.

Something that is certainly missing is a healthy dose of javadocs. They are not necessary for the test class and the ListOrderedSet just needs a class-level javadocs, the methods are self-explanatory.

However, as you probably had to look in-depth into what DebugOverlay and MetricsMode do, I'd encourage you to add class-level and method-level javadocs to them.

One thing I'm not convinced about is your somewhat special handling of the "DefaultMetrics". It seems to make things more complicated this way. I'd be happy for their instantiation to occur in the initialize() function, but I don't quite see the advantage in wrapping them in MetricModes.setDefaultMetrics(). I do like the use of the register() method to do it though.

By the way could you have used a LinkedHashSet instead of a custom class?

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger May 12, 2016

Member

The code looks good to me in general. A few notes on the details:

I'd also go for a LinkedHashSet, if possible. Afaics, the index-based access is not required, since the modes are just cycled continuously. Maybe Iterators.cycle() can be useful here?

I don't like the idea of passing this from the DebugOverlay to the different metrics so they can register. How about this: A new DebugRenderingSystem that is annotated with @Share and the debug overlay just renders its content? Modules and engine can then use the @In annotation to inject this system and register with it. This would be very flexible, but you need to create the metric instances through other means - maybe @RegisterSystem and/or implement RenderSystem.

Member

msteiger commented May 12, 2016

The code looks good to me in general. A few notes on the details:

I'd also go for a LinkedHashSet, if possible. Afaics, the index-based access is not required, since the modes are just cycled continuously. Maybe Iterators.cycle() can be useful here?

I don't like the idea of passing this from the DebugOverlay to the different metrics so they can register. How about this: A new DebugRenderingSystem that is annotated with @Share and the debug overlay just renders its content? Modules and engine can then use the @In annotation to inject this system and register with it. This would be very flexible, but you need to create the metric instances through other means - maybe @RegisterSystem and/or implement RenderSystem.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 12, 2016

Contributor

@msteiger: Whoa. As soon as somebody pops decorators all over the place I start feeling dizzy. =)

So, @Share-ing a class is what allows other classes to get an instance of it via the @In decorator of a member variable?

And what do you think of the previous way of doing this, in which metrics were instantiated directly in DebugOverlay? That wouldn't require passing the DebugOverlay instance anywhere.

Contributor

emanuele3d commented May 12, 2016

@msteiger: Whoa. As soon as somebody pops decorators all over the place I start feeling dizzy. =)

So, @Share-ing a class is what allows other classes to get an instance of it via the @In decorator of a member variable?

And what do you think of the previous way of doing this, in which metrics were instantiated directly in DebugOverlay? That wouldn't require passing the DebugOverlay instance anywhere.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 13, 2016

Member

So, @Share-ing a class is what allows other classes to get an instance of it via the @In decorator of a member variable?

Correct :-) @RegisterSystem tells the engine about it, @Share tells the engine to tell others about it, if they care to @In it - at least that's my understanding

Member

Cervator commented May 13, 2016

So, @Share-ing a class is what allows other classes to get an instance of it via the @In decorator of a member variable?

Correct :-) @RegisterSystem tells the engine about it, @Share tells the engine to tell others about it, if they care to @In it - at least that's my understanding

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 14, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 14, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes May 14, 2016

Member

@manu3d: One thing I'm not convinced about is your somewhat special handling of the "DefaultMetrics". It seems to make things more complicated this way.
@msteiger: A new DebugRenderingSystem that is annotated with @share and the debug overlay just renders its content?

I had no experience on annotations so far in Java. It seems very handy. I created a DebugRenderingSystem class that registers and shares itself. It would be possible for any third party developer to modify these metric modules.

@manu3d: By the way could you have used a LinkedHashSet instead of a custom class?

I wasn't sure about adding ListOrderedSet. Yeah, seems like we don't need index-based access. My last commit replaces and removes ListOrderedSet with LinkedHashSet.

One thing that I am not sure is the life-cycle of ui items and metric modules and totally adding a very custom ui item for displaying for instance a graph.

Member

tdgunes commented May 14, 2016

@manu3d: One thing I'm not convinced about is your somewhat special handling of the "DefaultMetrics". It seems to make things more complicated this way.
@msteiger: A new DebugRenderingSystem that is annotated with @share and the debug overlay just renders its content?

I had no experience on annotations so far in Java. It seems very handy. I created a DebugRenderingSystem class that registers and shares itself. It would be possible for any third party developer to modify these metric modules.

@manu3d: By the way could you have used a LinkedHashSet instead of a custom class?

I wasn't sure about adding ListOrderedSet. Yeah, seems like we don't need index-based access. My last commit replaces and removes ListOrderedSet with LinkedHashSet.

One thing that I am not sure is the life-cycle of ui items and metric modules and totally adding a very custom ui item for displaying for instance a graph.

@Cervator

This comment has been minimized.

Show comment
Hide comment
@Cervator

Cervator May 16, 2016

Member

@msteiger & @emanuele3d are the two of you OK with the code now to where we can merge or do we need more tweaks? :-)

Member

Cervator commented May 16, 2016

@msteiger & @emanuele3d are the two of you OK with the code now to where we can merge or do we need more tweaks? :-)

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 16, 2016

Contributor

@Cervator: no sorry, I need some more time to review it.

Contributor

emanuele3d commented May 16, 2016

@Cervator: no sorry, I need some more time to review it.

@@ -37,7 +36,6 @@
import org.terasology.world.biomes.Biome;
import org.terasology.world.biomes.BiomeManager;
import java.util.List;
import java.util.Locale;
/**

This comment has been minimized.

@emanuele3d

emanuele3d May 17, 2016

Contributor

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.

@emanuele3d

emanuele3d May 17, 2016

Contributor

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.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 19, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 19, 2016

Hooray Jenkins reported success with all tests good!

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 21, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 21, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 21, 2016

Contributor

I can't seem to find a way to reply immediately after your comment containing:

LinkedHashSet was a good candidate, however generating a new iterator and resetting toggle-order after every modification seems not the best way to do it. I know that proposing a new data structure without bugs is not easy. Therefore, I wrote unit tests for both DebugMetricsSystem and CircularToggleSet and tested in various scenarios.

So, I'll reply here.

I'd like to ask @msteiger his opinion about this. Originally the structure holding the MetricsMode instances was a simple list. Indeed using a plain list did not prevent registering the same instance multiple times, but the register() method could easily enforce that, by iterating over the list before something is added to it. This assumes that the list will never have enough items to run into performance issues - a safe assumption I believe.

In this context at this stage I do not see the utility of the new CircularToggleSet class. In fact, if register() enforced uniqueness, I don't even see the utility of a LinkedHashSet: the likely list size is too small to benefit from a hashed structure. It's late though. I might be missing something obvious.

Contributor

emanuele3d commented May 21, 2016

I can't seem to find a way to reply immediately after your comment containing:

LinkedHashSet was a good candidate, however generating a new iterator and resetting toggle-order after every modification seems not the best way to do it. I know that proposing a new data structure without bugs is not easy. Therefore, I wrote unit tests for both DebugMetricsSystem and CircularToggleSet and tested in various scenarios.

So, I'll reply here.

I'd like to ask @msteiger his opinion about this. Originally the structure holding the MetricsMode instances was a simple list. Indeed using a plain list did not prevent registering the same instance multiple times, but the register() method could easily enforce that, by iterating over the list before something is added to it. This assumes that the list will never have enough items to run into performance issues - a safe assumption I believe.

In this context at this stage I do not see the utility of the new CircularToggleSet class. In fact, if register() enforced uniqueness, I don't even see the utility of a LinkedHashSet: the likely list size is too small to benefit from a hashed structure. It's late though. I might be missing something obvious.

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes May 21, 2016

Member

CircularToggleSet class' advantage over 'LinkedHashSet' is preserving the position of the iterator, even if there is a modification during iteration. It provides same other properties as 'LinkedHashSet': uniqueness and insert-ordered. With CircularToggleSet, I tried to make data structure have all these previously mentioned properties and efficient at the same time. It's also true that a simple List might suffice for resolving our dilemma here.

Member

tdgunes commented May 21, 2016

CircularToggleSet class' advantage over 'LinkedHashSet' is preserving the position of the iterator, even if there is a modification during iteration. It provides same other properties as 'LinkedHashSet': uniqueness and insert-ordered. With CircularToggleSet, I tried to make data structure have all these previously mentioned properties and efficient at the same time. It's also true that a simple List might suffice for resolving our dilemma here.

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger May 22, 2016

Member

Using a CopyOnWriteArrayList should allow you to reuse the iterator, preserving the position.

Member

msteiger commented May 22, 2016

Using a CopyOnWriteArrayList should allow you to reuse the iterator, preserving the position.

@tdgunes

This comment has been minimized.

Show comment
Hide comment
@tdgunes

tdgunes May 22, 2016

Member

From java documentation: The "snapshot" style iterator method uses a reference to the state of the array at the point that the iterator was created. This array never changes during the lifetime of the iterator, so interference is impossible and the iterator is guaranteed not to throw ConcurrentModificationException. The iterator will not reflect additions, removals, or changes to the list since the iterator was created.

@msteiger: It seems ConcurrentModificationException will not thrown, but additions and removals after creation of iterator will not be visible or is there something that I am missing?

Member

tdgunes commented May 22, 2016

From java documentation: The "snapshot" style iterator method uses a reference to the state of the array at the point that the iterator was created. This array never changes during the lifetime of the iterator, so interference is impossible and the iterator is guaranteed not to throw ConcurrentModificationException. The iterator will not reflect additions, removals, or changes to the list since the iterator was created.

@msteiger: It seems ConcurrentModificationException will not thrown, but additions and removals after creation of iterator will not be visible or is there something that I am missing?

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 22, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 22, 2016

Hooray Jenkins reported success with all tests good!

@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger May 22, 2016

Member

That is correct. The Iterators.cycle() will request a new iterator when a cycle is complete, though. Thus, the changes will be visible in the next cycle.

Member

msteiger commented May 22, 2016

That is correct. The Iterators.cycle() will request a new iterator when a cycle is complete, though. Thus, the changes will be visible in the next cycle.

@emanuele3d

This comment has been minimized.

Show comment
Hide comment
@emanuele3d

emanuele3d May 22, 2016

Contributor

@tdgunes: I've added a number of comments but somehow they are listed as comments to an outdated diff. Please make sure to read them! =)

Contributor

emanuele3d commented May 22, 2016

@tdgunes: I've added a number of comments but somehow they are listed as comments to an outdated diff. Please make sure to read them! =)

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 23, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 23, 2016

Hooray Jenkins reported success with all tests good!

}
/**
* Returns current mode, initializes a default {@link NullMetricsMode}, if currentMode is null

This comment has been minimized.

@emanuele3d

emanuele3d May 24, 2016

Contributor

This javadoc is no longer valid. This method does not initialize a default NullMetricsMode. Just leave the @return line. Also, you might want to squash this and the next commit.

@emanuele3d

emanuele3d May 24, 2016

Contributor

This javadoc is no longer valid. This method does not initialize a default NullMetricsMode. Just leave the @return line. Also, you might want to squash this and the next commit.

This comment has been minimized.

@tdgunes

tdgunes May 24, 2016

Member

Oh, how did I miss this? Definitely, seems like I need to focus more on java docs, in my future commits.

@tdgunes

tdgunes May 24, 2016

Member

Oh, how did I miss this? Definitely, seems like I need to focus more on java docs, in my future commits.

This comment has been minimized.

@emanuele3d

emanuele3d May 24, 2016

Contributor

Definitely, seems like I need to focus more on java docs, in my future commits.

My fault. I'm a sucker for documentation. I hope @msteiger will keep me in check. However, do consider that some of these javadocs might be consulted in a webpage, without the source code in sight, hence the need for additional details.

@emanuele3d

emanuele3d May 24, 2016

Contributor

Definitely, seems like I need to focus more on java docs, in my future commits.

My fault. I'm a sucker for documentation. I hope @msteiger will keep me in check. However, do consider that some of these javadocs might be consulted in a webpage, without the source code in sight, hence the need for additional details.

*
* @return True if the MetricsMode instance was in the set. False otherwise.
*/
public boolean unregister(MetricsMode mode) {

This comment has been minimized.

@emanuele3d

emanuele3d May 24, 2016

Contributor

In the previous comment regarding this method I suggested to add to the javadoc the exceptions this method throws. There's also this form of method definition that includes the exceptions thrown:

public boolean unregister(MetricsMode mode) throws except1, except2 {

I'm not sure when to do it only in the javadoc, when to do it only in the method definition and when to do it both ways. @msteiger: Maaaaaaaaaartin?!?!?!?

By the way, the same applies to the register method.

@emanuele3d

emanuele3d May 24, 2016

Contributor

In the previous comment regarding this method I suggested to add to the javadoc the exceptions this method throws. There's also this form of method definition that includes the exceptions thrown:

public boolean unregister(MetricsMode mode) throws except1, except2 {

I'm not sure when to do it only in the javadoc, when to do it only in the method definition and when to do it both ways. @msteiger: Maaaaaaaaaartin?!?!?!?

By the way, the same applies to the register method.

@GooeyHub

This comment has been minimized.

Show comment
Hide comment
@GooeyHub

GooeyHub May 25, 2016

Member

Hooray Jenkins reported success with all tests good!

Member

GooeyHub commented May 25, 2016

Hooray Jenkins reported success with all tests good!

@msteiger msteiger merged commit 288ecc1 into MovingBlocks:develop May 26, 2016

1 check passed

default Build finished. 495 tests run, 0 skipped, 0 failed.
Details
@msteiger

This comment has been minimized.

Show comment
Hide comment
@msteiger

msteiger May 26, 2016

Member

Thanks, Taha!

Member

msteiger commented May 26, 2016

Thanks, Taha!

@GooeyHub GooeyHub removed the in progress label May 26, 2016

@Cervator Cervator added this to the v1.0.1 milestone May 28, 2016

@Cervator Cervator added the UI label May 28, 2016

@tdgunes tdgunes changed the title from [Ready for Review] Open up the DebugOverlay to add/remove MetricsModes to Open up the DebugOverlay to add/remove MetricsModes Jun 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment