Builder#648 support Widget Monitor #306

JiepengTan · 2024-07-25T09:33:16Z

Monitor.mp4

PS: The issues with font scaling and position adaptation will be resolved after the upgrade of the font library to EbitenEngine or Godot. Currently, go+builder can disable the Monitor's scaling feature to circumvent this bug.

xushiwei · 2024-07-30T02:57:32Z

monitor.go

-// stageMonitor class.
-type stageMonitor struct {
+// MonitorWidget class.
+type MonitorWidget struct {


stageMonitor 这个名字是 scratch 这边来的，是否保留兼容比较好？

另外不见得所有 widget 的名字都要带上 widget 吧？我个人其实喜欢不带 widget 字样的，比如 timer，不一定非要叫 timerWidget。

The name MonitorWidget comes from the requirements document: spx: support Widget Monitor builder#648.

I prefer to use a single module for all Widget types, named something like widget.Monitor. However, the implementation of Monitor depends on data structures such as drawContext. These data structures are placed in the top-level namespace. Extracting these structures into a separate general module to handle dependencies would require significant changes, so I kept the original design and placed MonitorWidget in the top-level namespace as well.

stageMonitor 这个名字是 scratch 这边来的，是否保留兼容比较好？

As far as I know, stageMonitor is a internal concept of Scratch. Scratch users do not need to know that name. While in Go+ builder, we expose the concept "Monitor" as a widget type (by two ways: 1. text "Monitor" in the Builder UI 2. MonitorWidget in code to do type-assertion). So the simplicity & intelligibility matters for its name here, and I don't think "stageMonitor" is a good name for this Go+ Builder widget type.

If we need to keep compatibility, I'll prefer to support both "stageMonitor" & "monitor", the first for compatibility & the second for Go+ Builder.

另外不见得所有 widget 的名字都要带上 widget 吧？我个人其实喜欢不带 widget 字样的，比如 timer，不一定非要叫 timerWidget。

Monitor & MonitorWidget are both fine for me. @xushiwei If you prefer Monitor here, we can name it Monitor.

nighca

As mentioned in https://github.com/goplus/spx/pull/306/files#r1703607442

Let's rename MonitorWidget to Monitor. I'll update the product doc as well.

nighca · 2024-08-06T07:01:56Z

game.go

+			return widget
+		}
+	}
+	return nil


It panics if not found.

We expect it to panic here, to keep consistent with other similar operations, e.g. turnTo("not_exist")

nighca · 2024-08-06T07:06:21Z

monitor.go

@@ -59,9 +61,14 @@ type stageMonitor struct {
 "isDiscrete": true,
 "visible": true
 */
-func newStageMonitor(g reflect.Value, v specsp) (*stageMonitor, error) {
+func newMonitorWidget(g reflect.Value, v specsp) (*MonitorWidget, error) {


It should be ok to omit color in config (v here), as it makes no sense for current monitor style. While it panics if I do so

panic: color is nil

nighca · 2024-08-06T07:14:20Z

monitor.go

 		}
-		w := labelW + textRectW + (stmHoriGapSm * 3)
-		h += (stmVertGapSm * 2)
+		hGap := stmHoriGapSm * p.size


There's issue with style details here. For example:

The gap size and rect height is incorrect

The label color is not correct

There should be no out border for the round-rect

There should be background of #fff

The background of value-part is translucent, which is not expected

There should be no border of round-rect of value-part

The width of value-part is not right

The issues related to font size, font changing, and layout adaptation will be resolved after the font library implementation is upgraded.

nighca · 2024-08-06T07:20:28Z

monitor.go

@@ -163,24 +171,30 @@ func (p *stageMonitor) draw(dc drawContext) {
 	default:
 		x, y := int(p.x), int(p.y)
 		labelRender := gdi.NewTextRender(defaultFont2, 0x80000, 0)
+		labelRender.Scale = p.size
 		labelRender.AddText(p.label)
 		labelW, h := labelRender.Size()

 		textRender := gdi.NewTextRender(defaultFontSm, 0x80000, 0)


The font size for value-part is not correct, 12px (maybe defaultFont2?) expected, while got 11px (defaultFontSm)

nighca · 2024-08-06T07:24:10Z

monitor.go

@@ -163,24 +171,30 @@ func (p *stageMonitor) draw(dc drawContext) {
 	default:
 		x, y := int(p.x), int(p.y)
 		labelRender := gdi.NewTextRender(defaultFont2, 0x80000, 0)
+		labelRender.Scale = p.size
 		labelRender.AddText(p.label)
 		labelW, h := labelRender.Size()

 		textRender := gdi.NewTextRender(defaultFontSm, 0x80000, 0)


And is it possible to specify font file? Or should we wait for

the upgrade of the font library to EbitenEngine or Godot.

and then support it?

qiniu-x · 2024-08-07T02:37:24Z

[Git-flow] Hi @JiepengTan, There are some suggestions for your information:

Rebase suggestions

Following commits have duplicated messages

Update monitor style

Update monitor style

Which seems insignificant, recommend to use git rebase command to reorganize your PR.

For other git-flow instructions, recommend refer to these examples.

If you have any questions about this comment, feel free to raise an issue here:

https://github.com/qiniu/reviewbot

nighca · 2024-08-07T08:28:24Z

monitor.go

@@ -75,8 +81,8 @@ func newStageMonitor(g reflect.Value, v specsp) (*stageMonitor, error) {
 	x := v["x"].(float64)


In the current implementation, for widgets' position, x: 0, y: 0 means the left-top corner (of viewport); while for sprites' position, x: 0, y: 0 means the center point (of map).

Though the positioning behavior of widgets and sprites is different: widgets are positioned relative to the viewport, and sprites are positioned relative to the map. I believe it'll be better to keep the convention that x: 0, y: 0 means the center point for both.

@JiepengTan @xushiwei What do you think?

And the direction of the coordinate system. Currently, for widgets, the Y-axis points down, while for sprites, the Y-axis points up; I suggest we unify this as well.

nighca · 2024-08-09T08:52:30Z

monitor.go

@@ -163,24 +170,30 @@ func (p *stageMonitor) draw(dc drawContext) {
 	default:
 		x, y := int(p.x), int(p.y)
 		labelRender := gdi.NewTextRender(defaultFont2, 0x80000, 0)
+		labelRender.Scale = p.size


There are two different implementations for TextRender here:

internal/gdi/gdi.go, which will be used for build without -tags canvas

internal/gdi/gdi_canvas.go, which will be used for build with -tags canvas

In this PR the field Scale is added in the first implementation only.

So if we run

GOOS=js GOARCH=wasm go build -tags canvas

to build spx, it throws error:

# github.com/goplus/spx ./monitor.go:173:15: labelRender.Scale undefined (type gdi.TextRender has no field or method Scale) ./monitor.go:178:14: textRender.Scale undefined (type gdi.TextRender has no field or method Scale)

In web page of Builder, we use spx version which is built with -tags canvas to render within HTML Canvas.

To fix that, I think we should add field Scale in the implementation for TextRender in internal/gdi/gdi_canvas.go as well.

nighca · 2024-08-09T10:15:52Z

monitor.go

 	return
 }

 // -------------------------------------------------------------------------------------
+// IWidget
+func (pself *Monitor) GetName() string {


Here it is *Monitor instead of Monitor who implemented interface Widget. Hence the user will have to do type assertion like this:

scoreMonitor := getWidget("life").(*Monitor)

To understand the code, the user will have to know the concept "pointer", which I think is not expected. To avoid the user noticing concept "pointer", I'll suggest to rename type Monitor struct to something like MonitorImpl and make a type alias for *MonitorImpl:

type MonitorImpl struct {} func (pself *MonitorImpl) Visible() bool {} func (pself *MonitorImpl) Show() {} func (pself *MonitorImpl) Hide() {} type Monitor = *MonitorImpl

Then the user can do type assertion with Monitor without noticing pointer:

scoreMonitor := getWidget("life").(Monitor)

As discussed, we will support

// generics is utilized under the hood scoreMonitor := getWidget(Monitor, "life")

rather than

scoreMonitor := getWidget("life").(Monitor)

nighca · 2024-08-12T01:40:57Z

Remaining issues

Remaining issues will be addressed in separate PRs

Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 fix widget #310
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 Convert the "GetWidget" function to a generic function #311
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 fix widget #310
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 Convert the "GetWidget" function to a generic function #311
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 Convert the "GetWidget" function to a generic function #311
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 fix widget #310
Builder#648 support Widget Monitor #306 (comment) fix by Builder#648 Convert the "GetWidget" function to a generic function #311

JiepengTan added 3 commits July 25, 2024 15:35

support Widget Monitor

d78c2bb

Update monitor style

c2b627c

Support scale monitor

35ba6e4

JiepengTan requested a review from nighca July 25, 2024 09:33

nighca mentioned this pull request Jul 26, 2024

Widget Monitor goplus/builder#676

Merged

2 tasks

xushiwei reviewed Jul 30, 2024

View reviewed changes

nighca reviewed Aug 6, 2024

View reviewed changes

Rename MonitorWidget => Monitor

55d71d4

nighca mentioned this pull request Aug 7, 2024

spx: font issues goplus/builder#710

Open

Update monitor style

2e32fae

xushiwei merged commit ccef49d into goplus:main Aug 7, 2024
3 checks passed

nighca reviewed Aug 7, 2024

View reviewed changes

nighca reviewed Aug 9, 2024

View reviewed changes

JiepengTan mentioned this pull request Aug 15, 2024

Builder#648 Convert the "GetWidget" function to a generic function #311

Merged

JiepengTan deleted the builder#648 branch September 23, 2024 07:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Builder#648 support Widget Monitor #306

Builder#648 support Widget Monitor #306

JiepengTan commented Jul 25, 2024

xushiwei Jul 30, 2024

JiepengTan Jul 31, 2024

nighca Aug 5, 2024 •

edited

Loading

nighca left a comment

nighca Aug 6, 2024

nighca Aug 6, 2024

nighca Aug 6, 2024

JiepengTan Aug 7, 2024

nighca Aug 6, 2024 •

edited

Loading

nighca Aug 6, 2024 •

edited

Loading

qiniu-x bot commented Aug 7, 2024

nighca Aug 7, 2024

nighca Aug 7, 2024 •

edited

Loading

nighca Aug 9, 2024 •

edited

Loading

nighca Aug 9, 2024

nighca Aug 12, 2024 •

edited

Loading

nighca commented Aug 12, 2024 •

edited

Loading

		@@ -75,8 +81,8 @@ func newStageMonitor(g reflect.Value, v specsp) (*stageMonitor, error) {
		x := v["x"].(float64)

Builder#648 support Widget Monitor #306

Builder#648 support Widget Monitor #306

Conversation

JiepengTan commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

nighca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

nighca Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

qiniu-x bot commented Aug 7, 2024

Rebase suggestions

Choose a reason for hiding this comment

nighca Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

nighca Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nighca Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

nighca commented Aug 12, 2024 • edited Loading

Remaining issues

nighca Aug 5, 2024 •

edited

Loading

nighca Aug 6, 2024 •

edited

Loading

nighca Aug 6, 2024 •

edited

Loading

nighca Aug 7, 2024 •

edited

Loading

nighca Aug 9, 2024 •

edited

Loading

nighca Aug 12, 2024 •

edited

Loading

nighca commented Aug 12, 2024 •

edited

Loading