redesign the help statistics plot by Neil-Tomar · Pull Request #560 · Java-Discord/JavaBot

Neil-Tomar · 2026-09-06T12:13:44Z

redesign the help statistics plot.

danthe1st

Don't worry too much about updating the test graph while it is still in review, I will give you one once we are at a point where the PR is otherwise ok.
The differences between Windows and Linux may be caused by differences in antialiasing.

danthe1st · 2026-09-06T12:48:32Z

-import java.util.LinkedHashMap;
-import java.util.List;
-import java.util.Map;
+import java.util.*;


Please avoid the wildcard import.

Note: The wildcard import is still present.

danthe1st · 2026-09-06T12:59:44Z

+
+    public static boolean compareImage(BufferedImage img1, BufferedImage img2) {
+        if (img1 == null || img2 == null) {
+            return false;


Instead of just returning boolean and asserting it being true, please use proper assertions when they happen, e.g. assertNonNull(actual) and assertEquals(expected.getWidth(), actual.getWidth(), "image width does not match")

danthe1st · 2026-09-07T16:17:07Z

When I built it with native-image, it failed with the following exception:

Exception in thread "main" java.lang.NoSuchFieldError: sun.java2d.pipe.ShapeSpanIterator.pData
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.functions.JNIFunctions$Support.getFieldID(JNIFunctions.java:1989)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.functions.JNIFunctions.GetFieldID(JNIFunctions.java:479)
        at java.desktop@25.0.4.1/sun.java2d.pipe.ShapeSpanIterator.initIDs(Native Method)
        at java.desktop@25.0.4.1/sun.java2d.pipe.ShapeSpanIterator.<clinit>(ShapeSpanIterator.java:71)
        at java.desktop@25.0.4.1/sun.java2d.pipe.LoopPipe.getStrokeSpans(LoopPipe.java:261)
        at java.desktop@25.0.4.1/sun.java2d.pipe.LoopPipe.draw(LoopPipe.java:195)
        at java.desktop@25.0.4.1/sun.java2d.pipe.PixelToShapeConverter.drawLine(PixelToShapeConverter.java:52)
        at java.desktop@25.0.4.1/sun.java2d.pipe.PixelToParallelogramConverter.drawLine(PixelToParallelogramConverter.java:81)
        at java.desktop@25.0.4.1/sun.java2d.pipe.ValidatePipe.drawLine(ValidatePipe.java:62)
        at java.desktop@25.0.4.1/sun.java2d.SunGraphics2D.drawLine(SunGraphics2D.java:2244)
        at net.discordjug.javabot.util.Plotter.drawLines(Plotter.java:136)
        at net.discordjug.javabot.util.Plotter.drawGraph(Plotter.java:122)
        at net.discordjug.javabot.util.Plotter.plot(Plotter.java:89)
        at net.discordjug.javabot.PlotMain.main(PlotMain.java:27)

danthe1st · 2026-09-07T16:38:10Z

Performance test (single shot, running only the plotting logic, including startup time, with native-image, repeated 6 times)
docker run --rm -it --entrypoint sh dan1st/javabot_plotter:before

~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.67s
user    0m 0.23s
sys     0m 0.03s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.56s
user    0m 0.22s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.51s
user    0m 0.23s
sys     0m 0.03s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.55s
user    0m 0.23s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.53s
user    0m 0.23s
sys     0m 0.03s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.62s
user    0m 0.23s
sys     0m 0.03s

docker run --rm -it --entrypoint sh dan1st/javabot_plotter:after

~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.59s
user    0m 0.22s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.60s
user    0m 0.24s
sys     0m 0.03s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.63s
user    0m 0.23s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.82s
user    0m 0.24s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.54s
user    0m 0.23s
sys     0m 0.04s
~ $ time /work/javabot -Djava.home=/tmp/JAVA_HOME
real    0m 0.60s
user    0m 0.23s
sys     0m 0.05s

It seems like at least with your test case, your change doesn't seem to make too much of a difference in terms of time.

danthe1st · 2026-09-07T16:43:46Z

This is the example data generated with your PR within the Docker image (except with a different title/subtitle).

danthe1st · 2026-09-27T14:23:59Z

+	 * @param subtitleText the subtitle of plot
 	 */
-	public Plotter(List<Pair<String, Bar>> entries, String title, String subtitle) {
+	public Plotter(List<Pair<String, Bar>> entries, String titleText, String subtitleText) {


Is this constructor needed (are there any calls that shouldn't set darkMode to anything)? If yes, is there a reason this doesn't just call the other constructor using this(entries, titleText, subtitleText, false);?

danthe1st · 2026-09-27T14:33:26Z

-						.mapToDouble(Bar::sum)
-						.max().orElse(0);
+	private void drawGraph(Graphics2D graphics2D) {
+		double maxValue = entries.stream().map(Pair::second).mapToDouble(Bar::sum).max().orElse(0);


Is there a reason you reduced this to a single line?

danthe1st · 2026-09-27T14:51:01Z

+	private static final int AXIS_LABEL_MARGIN_LEFT = (int) (WIDTH * 0.045);           // 4.5%
+	private static final int GRAPH_MARGIN_LEFT = (int) (WIDTH * 0.05);                 // 5%
+	private static final int GRAPH_MARGIN_RIGHT = (int) (WIDTH * 0.02);                // 2%
+	private static final int AXIS_LABEL_MARGIN_BOTTOM = (int) (HEIGHT * 0.043);        // 4.3%


As far as I can see, this is the amount of space between the bottom of the plot and the top of the label. Is this correct? I think there's a risk of confusing this something else (e.g. GRAPH_MARGIN_BOTTOM is the space between the very bottom of the image and the bottom of the graph as far as I can see which is the more obvious interpretation) so a comment saying this wouldn't hurt IMO.

Words are getting kind of confusing.

I believe AXIS_LABEL_MARGIN_BOTTOM is the space between red lines. but I just changed some things which will be in next commit so now bottom red line is shifted to middle of the text like in image below.

I know this is more confusing because it is supposed to represent space between. If you want, I can add extra value like FontHeight/2 at drawStringCentered(graphics2D, label, centerX, GRAPH_MARGIN_TOP + GRAPH_HEIGHT + AXIS_LABEL_MARGIN_BOTTOM);.
let me know on what you think.

danthe1st · 2026-09-27T14:58:23Z

+	private static final int GRID_LINES = 5;
+	private static final int BAR_LABEL_MARGIN = 34;
+	private static final int BAR_LABEL_HEIGHT = 42;
+	private static final int BAR_LABEL_BOTTOM_MARGIN = 65;


Similar to AXIS_LABEL_MARGIN_BOTTOM, I think a comment explaining what exactly this is wouldn't hurt either. From what I can see, this is the space between the bottom of the axis label and the top of the bar (which I think is intuitive but explaining it in a comment doesn't hurt either)?

BAR_LABEL_BOTTOM_MARGIN represents the space between red lines in following image.

I know this is confusing because once again this is supposed to represents space between Bar Label and Top Bar. The issue is caused because y-axis value increases when moving down and I got confused because of that. But the issue is fixed and will be committed in next change.

danthe1st · 2026-09-27T15:02:52Z

-				double value = element.second();
-				int segmentHeight = (int) (height * (value / axisMax));
-				if (segmentHeight <= 0) continue;
+		Font valueFont = ImageGenerationUtils.getResourceFont("assets/fonts/Uni-Sans-Heavy.ttf", VALUE_SIZE).orElseThrow();


I think you should only create the font once (per plot) and not for every label. You could move this to a field (and initialize it in the constructor or similar). I don't have a particular preference for whether or not static variables should be used but it may be a good idea to do the same for all fonts in the Plotter class.

danthe1st · 2026-09-27T15:12:43Z

+		graphics2D.setColor(barLabelColor);
+		graphics2D.fillRoundRect(centerX - barLabelWidth / 2, barLabelY, barLabelWidth, BAR_LABEL_HEIGHT, ARC_SIZE, ARC_SIZE);
+		graphics2D.setColor(textColor);
+		drawStringCentered(graphics2D,totalText,centerX, barLabelY + 29);


Maybe this magic number (29) should be moved to a constant?

danthe1st · 2026-09-27T15:20:56Z

+                assertEquals(actualImage.getRGB(x, y),expectedImage.getRGB(x, y),() -> "Image does not match.\nActual image:"+actualImageBase64);
            }
        }
        return true;


I don't think there's a reason to return anything here. Just make it void.

danthe1st · 2026-09-27T15:22:00Z

+
+        for (int y = 0; y < actualImage.getHeight(); y++) {
+            for (int x = 0; x < actualImage.getWidth(); x++) {
+                assertEquals(actualImage.getRGB(x, y),expectedImage.getRGB(x, y),() -> "Image does not match.\nActual image:"+actualImageBase64);


I think it would be useful to include the coordinates in the message here.

danthe1st · 2026-09-27T15:26:20Z

-        for (int i = 0; i < 13; i++) {
-            int segments = random.nextInt(3,5);
-            List<Pair<Color, Double>> parts = new ArrayList<>();
+    private static List<Pair<String, Plotter.Bar>> testData0() {


I think having descriptive names for the test data methods would be useful. I guess you can just call this testData (or createTestData) and then use names like testDataSmall and createEmptyTestData or similar.

danthe1st · 2026-09-27T15:28:14Z

-        for (int i = 0; i < 13; i++) {
-            int segments = random.nextInt(3,5);
-            List<Pair<Color, Double>> parts = new ArrayList<>();
+    private static List<Pair<String, Plotter.Bar>> testData0() {


I also think it would be good to have a bar with no segments in one of the test data methods.

redesign the help statistics plot

f5aa49f

Neil-Tomar requested a review from a team as a code owner September 6, 2026 12:13

danthe1st requested changes Sep 6, 2026

View reviewed changes

danthe1st reviewed Sep 6, 2026

View reviewed changes

Comment thread src/test/java/net/discordjug/javabot/util/PlotterTest.java Outdated

fixed requested changes

f5a6ff8

danthe1st requested changes Sep 27, 2026

View reviewed changes

Conversation

Neil-Tomar commented Sep 6, 2026

Uh oh!

danthe1st left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danthe1st commented Sep 7, 2026

Uh oh!

danthe1st commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danthe1st commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danthe1st Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

danthe1st left a comment •

edited

Loading

danthe1st commented Sep 7, 2026 •

edited

Loading

danthe1st commented Sep 7, 2026 •

edited

Loading

danthe1st Sep 27, 2026 •

edited

Loading