WIP: cops-ified view system #31

jcorbin · 2017-12-14T07:10:19Z

No description provided.

jcorbin · 2017-12-14T07:11:40Z

Mostly past lint, only deathroom remains; then onto the view test failures.

The difficult part is the view.Grid => display.Display conversion...

kriskowal · 2017-12-15T01:47:02Z

internal/cops/display/display.go

@@ -92,6 +98,30 @@ func (d *Display) SetRGBA(x, y int, t string, f, b color.RGBA) {
 	}
 }

+// MergeRGBA sets the given string and colors if they are
+// non-empty and not transparent respectively.


For least surprise, I expect this method to be behaviorally equivalent to:

mini := display.New(image.Rect(0, 0, 1, 1)) mini.Set(0, 0, t, f, b) display.Draw(d, image.Rect(x, y, x + 1, y + 1), mini, image.ZP, draw.Over)

That accounts for the opacity of the foreground and background, over the underlying foreground and background, replacing text only if there’s text. Would be a good way to validate this method.

The tricky part is nontrivial alpha values. It is certainly possible to borrow the algorithm from "image/draw". See the inner loop of drawNRGBAOver.

I’m fine with adding the method with a partial implementation, provided it can grow into this behavior.

kriskowal · 2017-12-15T01:53:46Z

internal/cops/display/display.go

+		if t != "" {
+			d.Text.Strings[i] = t
+		}
+		if f.A < 0xff {


I believe you mean to do full replacement when the opacity is 1.0, not less than 1.0, do nothing when opacity is 0. I would like to go all the way and implement blending for intermediate values.

I’ll note that display.Draw takes into account the non-obvious case of blending the top background color over the underlying foreground, which causes the underlying text to fade into the background. See the hicops example for the visual effect.

draw.Draw(dst.Background, r, src.Background, sp, op) draw.Draw(dst.Foreground, r, src.Background, sp, op) draw.Draw(dst.Foreground, r, src.Foreground, sp, op) textile.Draw(dst.Text, r, src.Text, sp)

kriskowal · 2017-12-15T01:55:27Z

internal/cops/display/display.go

+// returning how many cells were affected.
+//
+// NOTE does not support multi-rune glyphs
+func (d *Display) WriteString(x, y int, f, b color.Color, mess string, args ...interface{}) int {


I’ve used msg elsewhere, stylewise. Three letter abbreviations are emerging as an aesthetic with this library, buf, cur, dis (now that we don’t distinguish front and back in usage).

kriskowal · 2017-12-15T01:56:16Z

internal/cops/display/display.go

+// string and the diplay).
+//
+// NOTE does not support multi-rune glyphs
+func (d *Display) WriteStringRTL(x, y int, f, b color.Color, mess string, args ...interface{}) int {


I think you might mean RightAlign. RTL implies the Semitic mode.

kriskowal · 2017-12-15T01:58:16Z

internal/cops/display/display.go

+
+// Lines returns a slice of row strings from the display, filling in any zero
+// runes with the given one.
+func (d *Display) Lines(fillZero string) []string {


Is fillZero ever not space? I can only imagine the case where a display is composited over a background of uniform text.

Yeah, this is used only as a test convenience utility; so maybe will punt it to an ancillary test package. In testing circumstances, I have occasionally found it useful to fill with some obvious glyph to differentiate "explicit space".

kriskowal · 2017-12-15T01:59:24Z

internal/cops/terminal/terminal.go

@@ -19,6 +19,8 @@ type Terminal struct {

 // New returns a Terminal for the given file descriptor, capable of restoring
 // that terminal to its current state.
+//
+// FIXME New() *T; value can be Make() T or T.Init()


I’ll take *T.

yeah, the T.Init() form is chiefly useful when embedding is your use case; e.g. how internal/ecs makes heavy use of that convention.

kriskowal · 2017-12-17T02:59:25Z

internal/view/view.go

@@ -69,7 +68,7 @@ func (dm *displayModel) Close() error {
 func (dm *displayModel) updateSize() error {
 	bounds, err := dm.term.Bounds()
 	if err == nil {
-		dm.Display = display.New(bounds)
+		dm.dis = display.New(bounds)


What do you think of a (*Display)Resize(Rectangle)*Display method that reallocates if necessary, reuses if possible, on the append precedent?

would love it, meant to drop a TODO about it; only been avoiding due to scope

kriskowal · 2017-12-17T03:14:14Z

internal/view/view.go

-		dm.buf = dm.buf[:copy(dm.buf[:m], dm.buf[n:])]
-	} else {
-		dm.buf = dm.buf[:0]
+	for attempts > 1 && err == io.ErrShortWrite {


Ah, look what you found :|

yeah, once I get through this PR I want to holistically benchmark rendering, evaluating things like:

whether isolating Display => buf rendering from client rendering and input processing (by putting it in a dedicated goroutine and renditioning displays with it is worth it ; probably)

whether / when differential rendeing is worth it from an stime / syscall perspective vs the couple millisecond utime hit; especially once that couple ms is parallelizable per prior

I ought to take this and see if it fixes whole pages of family of four.

jcorbin · 2017-12-19T17:27:06Z

Remaining display warts:

world grid smearing between frames; not even sure if it's a rendering or display drawing problem
something's still broken with color handling as you can see with Add a testpattern proof #37

jcorbin requested a review from kriskowal December 14, 2017 07:11

jcorbin force-pushed the cops_view branch from ede2f05 to eb9a4b8 Compare December 14, 2017 17:18

kriskowal reviewed Dec 15, 2017

View reviewed changes

jcorbin force-pushed the cops_view branch from af5c960 to 176a406 Compare December 17, 2017 03:15

kriskowal reviewed Dec 17, 2017

View reviewed changes

This was referenced Dec 17, 2017

cops: use struct for TIOC[GS]WINSZ #32

Merged

Add cops cell merging #34

Open

Trying to validate basic display rendering #35

Open

jcorbin force-pushed the cops_view branch from 176a406 to 81f60fe Compare December 17, 2017 04:47

This was referenced Dec 17, 2017

Rework the input channel to be less tightly coupled #30

Closed

Factor out cops display.Terminal #36

Merged

jcorbin force-pushed the cops_view branch from 81f60fe to 52c473e Compare December 18, 2017 05:29

jcorbin mentioned this pull request Dec 18, 2017

Add a cops draw test #39

Merged

jcorbin force-pushed the cops_view branch 2 times, most recently from 20df38f to f7e34da Compare December 19, 2017 17:22

jcorbin added 11 commits December 28, 2017 10:30

Add a test pattern proof

2fb34e7

cops: Display.Set: support nil colors

aa429d8

cops: add Display.MergeRGBA

de4e1d2

cops: break out display rendering module

d4a92d9

Rework the input channel to be less tightly coupled

d7e706c

cops: add Display.WriteString(RTL)

4c453c3

FIXME New() Terminal

6e37c11

WIP: internal/view: cops-ify

8c234f8

DEBUG log in view demo

b0a6fae

WIP: demo test pattern

725270a

FIX hud world centering

13611d8

jcorbin force-pushed the cops_view branch from f7e34da to 13611d8 Compare December 28, 2017 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: cops-ified view system #31

WIP: cops-ified view system #31

jcorbin commented Dec 14, 2017

jcorbin commented Dec 14, 2017

kriskowal Dec 15, 2017

kriskowal Dec 15, 2017

kriskowal Dec 15, 2017

kriskowal Dec 15, 2017

kriskowal Dec 15, 2017

jcorbin Dec 17, 2017

kriskowal Dec 15, 2017

jcorbin Dec 17, 2017

kriskowal Dec 17, 2017

jcorbin Dec 17, 2017

kriskowal Dec 17, 2017

jcorbin Dec 17, 2017

kriskowal Dec 17, 2017

jcorbin commented Dec 19, 2017

WIP: cops-ified view system #31

Are you sure you want to change the base?

WIP: cops-ified view system #31

Conversation

jcorbin commented Dec 14, 2017

jcorbin commented Dec 14, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcorbin commented Dec 19, 2017