-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Add VGA graphical text mode #1123
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I've left a few stylistic comments. However, looking at the bigger picture, many of the changes in vga.js look similar to this:
- this.bus.send("screen-update-cursor-scanline", [start, end, visible]);
+ if(this.graphical_text)
+ {
+ this.graphical_text.set_cursor_attr(start, end, visible);
+ }
+ else
+ {
+ this.bus.send("screen-update-cursor-scanline", [start, end, visible]);
+ }
Which indicates that it might be better to merge src/vga_text.js into src/browser/screen.js. It would also be nice to reuse the screen-put-char
machinery.
src/vga.js
Outdated
this.bus.send("screen-set-mode", graphical_mode || this.graphical_text); | ||
} | ||
|
||
VGAScreen.prototype.grab_text_content = function(keep_whitespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early on you mentioned the current ability to select text from the DIV using the mouse.
This is my poor-man's replacement for this, but I left out the accompanying method in V86's user interface, currently I call it using v86.cpu.devices.vga.grab_text_content() in my V86 embedding.
I could add mouse-controlled text-selection though, roughly like this:
- Text selection is simply a linear copy from the 16 bit VGA text buffer, starting at (start-row, start-col) to (end-row, end-col), just a VGA memory range between a start- and an end-address.
- If any character within the current selection is changed, selection gets cancelled and reset to nothing.
- Swap forground and background color when rendering selected text.
- Immediately copy to clipboard when user changes selected text range.
- Any events from mouse and keyboard related to text selection must be hidden from the virtualized OS. Not sure yet how to modify MouseAdapter and KeyboardAdapter here.
But someone would have to establish a mouse-button + key combination that triggers text selection in graphical text mode. For example, I use the DOS mouse driver in FreeDOS, and I can select/copy text within FreeDOS's EDIT.EXE using my mouse. So the normal left mouse button is not an option, text-based applications may use it in graphical text mode. IIRC already DOS supported mice with 3 buttons, and certainly the 104/105 keyboards.
Or, make text selection in graphical text mode a special state that has to be triggered through the HTML user interface, naively like a button "Begin Text Selection" which changes its label to "End Text Selection" when clicked, and during that state the left mouse button would be used for text selection normally and all mouse events would be hidden from the virtualized OS. I'd prefer that.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add mouse-controlled text-selection though, roughly like this:
This sounds reasonable. I would change the following:
If any character within the current selection is changed, selection gets cancelled and reset to nothing.
In my experience, this makes copying regularly changing text annoying, and the alternative is better.
Immediately copy to clipboard when user changes selected text range.
I don't think this is a good idea, as users expect the "browser-like" behaviour of copying with ctrl+c. It's also likely that browser's don't allow changing the clipboard without user interaction.
An alternative implementation could use regular DOM text elements with the current vga text implementation, that are "above" the canvas screen but opaque. I believe xterm.js does something like that, but I can't find the implementation right now.
Or, make text selection in graphical text mode a special state that has to be triggered through the HTML user interface, naively like a button "Begin Text Selection" which changes its label to "End Text Selection" when clicked, and during that state the left mouse button would be used for text selection normally and all mouse events would be hidden from the virtualized OS. I'd prefer that.
That sounds reasonable (much better than a mouse-button + key combination), and it's also similar how text selection in the existing text screen works: When the guest OS uses the mouse, clicking on the screen doesn't select text, it locks the mouse. When the user clicks on "disable mouse" (or the guest OS doesn't use the mouse in the first place), text selection is provided by the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fully agree (explicit select/copy-state and Ctrl+C to copy).
The canvas overlay idea is good and I will look into it when I find time. Maybe one way to go would be to use context.fillRect() with context.globalCompositeOperation = "xor" to invert text in the browser canvas at low costs (as oppsed to the render() function of GraphicalText).
A HTML DIV overlay in graphical text mode would make the font appearance mismatch. It also touches on the issue of the dreaded 8-bit codepages and their Unicode-mappings.
However, I'd vote to leave this issue for another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this little demo I wrote about highlighting selected text in a canvas using xor. That on the top is a canvas with a static image, the raw text data is simply stored fixed in the js file. Highlighting is a purely graphical effect here, without using the raw text.
context.globalCompositeOperation isn't what it seems.
You're welcome, and thank you very much for taking the time for a close look at all this, I really appreciate that. I agree that the new code should be merged into existing classes, it is structured that way (minimal changes to existing code) to best oversee how. Here's where I ended up: In graphical text mode, the entire DIV-based text implementation in ScreenAdapter is currently dead on purpose as I did not see any use for it, the code is 100% "living off the land" in VGAScreen, specifically without the 2nd text buffer ScreenAdapter.text_mode_data[] and all the code that goes with it. Are there other bus listeners besides ScreenAdapter for any of the 4 text-related bus events screen-put-char, screen-update-cursor, screen-set-size-text and screen-update-cursor-scanline that need to be taken into consideration? Could you please describe a bit more to me what behaviour you would like to see restored? Currently, the only essential function of ScreenAdapter in graphical text mode is that it controls the rendering loop at ~60fps, faithfully believing it is in graphical mode (hehe). I haven't made up my mind, though I currently think GraphicalText (in vga_text.js) is much closer to VGAScreen than ScreenAdapter. Maybe GraphicalText could be split across VGAScreen and ScreenAdapter since its function does sit somewhere in between the two. The rendering should happen in VGAScreen, do you agree? I need to think about this, not sure where to draw the line. By the way, even though it is not used in graphical text mode, the HTML DIV element is still required because ScreenAdapter still assumes its existence, this could be easily changed as ScreenAdapter also knows about graphical text mode in its constructor through the screen_options constructor argument. Should I? |
Looking at this again, you're right that the line is blurry. I think we can merge this in its current design, and I'll think about potential refactoring later (see the next paragraph). Regarding bus, this is a leftover from a very old version of v86 that supported web workers (with the idea that "bus" was the boundary between web worker and main thread). It's now mostly used as an event handler for the public V86 class. I'm thinking of removing it and replacing it with more direct calls.
No, an invisible div doesn't harm anyone (and code that switches between modes might assume its existence). |
I will finish this PR today, there is only one last issue. It just occurred to me that I haven't thought about V86's persistent state[] at all, GraphicalText is currently completely ignorant about save/restore state. Later today I will look into this and it is very likely that I will push one last patch by the evening to fix this in case it is broken (which I believe it is). Whether or not, I'm determined to finish by this evening because I'll need the next few weeks for other things. Can you give me a quick sketch about what needs to be considered when saving/restoring state? |
Yesterday I took a very detailled look at this PR's changeset to ensure everyhing was ok (which it wasn't), and also took notes on the design for an overview. Here are the current relations between VGAScreen, ScreenAdapter and GraphicalText:
In detail (functions that suppress a bus message are annotated with the message's name):
I also cobbled a crude hack together where I changed the design to this:
This proof-of-concept worked, VGAScreen was left with almost no changes (compared to the current master) and it was as ignorant about GraphicalText as ScreenAdapter currently is in my PR. ScreenAdapter becomes the owner of the private GraphicalText instance (currently VGAScreen is the owner). There are only a few hurdles for this transformation, I guess it would take me a couple of hours to make it right and proper. As before, GraphicalText needs a valid reference to VGAScreen, but that's solvable. If this design issue comes up in the future I'd be happy to help. |
Never mind, I did a few tests and it seems to work as it is, allthough I'm not sure I'm testing right. I saved the state, reloaded my browser, rebooted using the same image and then restored the state. The screen restored to the exact same state (content and cursor) that I had saved. Then I changed mode from 25 to 50 rows, trashed the screen and restored state again. Also the second time the screen was restored perfectly including 25 line mode. I guess it works because I'm working off the VGA state which does get saved/restored properly. |
FYI, I'm reworking the bus interface, and will try to get this merged afterwards. |
Ok! The other day I made a thorough cross-check with qemu, and the text modes look and behave the same as in this PR. So from my perspective it's complete and ready to be merged. |
just found out yesterday there's another repo named doswasmx, seemingly had solutions to a few issues here. |
If I run a text mode application in DOS Wasm X which intensely dumps to stdout the screen gets quite unresponsive and kinda breaks (I ran nmake over the MS-DOS 4.0 source tree with a Pentium II at 300Mhz), see here: It's a bit hard to catch in a screenshot but take a look at the 3rd row from the bottom for what I mean. A bit worse, if I run the Norton Commander installer in DOS Wasm X I don't see the font-based graphical mouse-cursor emulation (see the 3rd screebshot and text below here), and then Firefox crashes. I only know a few simulators and thus don't have much of an opinion, but perhaps EM-DOSBox is a better wasm-port of DOSBox than "DOS Wasm X", EM-DOSBox runs all the simulations on archive.org, and those work quite well in my experience. See also here: The Secret Feature of EM-DOSBOX on Internet Archive. |
Indeed Firefox crashes running doswasmx. |
Merged in c758d6d and f92e6b4. I meant to push to wip instead of master, but oh well. There's still more refactoring I'd like to do, in particular:
However I have other things to work on. It would be nice if you could submit some of the refactorings as a PR, but it's good enough to be merged as is. Could you do a quick test to see if everything is still working? I moved several things around, especially around the font size checks. |
Can you undo this merge? |
Else I would like to begin with the state of vga.js in the 2nd-to-last commit c758d6d and just override what was then merged in the last commit f92e6b4. It looks good to me, would that be safe? I have a solid plan how to implement what you listed in one go, I'm only looking for a better starting position. Compared to the previous state of vga.js, I only need to add 2 methods ScreenAdapter.set_font_bitmap() and ScreenAdapter.set_font_page() which get called by VGAScreen (let me know if I should expand on them) and make sure that the 6 VGA register-additions I made are good, these are the only changes to vga.js, very few. For the rest, GraphicalText will get merged into ScreenAdapter, and ScreenAdapter won't need a link to VGAScreen. ScreenAdapter will be the only class with knowledge about graphical text mode. Graphical text mode implementation in ScreenAdapter will reuse as much as possible from classical text mode (all the methods (former bus messages), the character and attribute buffer, cursor attributes, etc.). All rendering (font and screen) will be implemented in ScreenAdapter. |
I can't, but feel free to include |
Ok I have started the 2nd iteration of this. Please do not modify vga.js or screen.js in the next couple of days. |
Success! I can see it booting as it's supposed to, so far I've tested: 50 row-mode, 8/9px font width and font pages A/B. Fontraption and Norton Commander look as they should. So the main part is working, only minor things like the cursor are left to do. |
- moved declarations of constants, member variables and methods into separate groups - moved scattered initialization code into init() method - removed redundant members and initialization code - refactored timer() method to simplify and clarify code - made control flow depend on three-state variable "mode" only
src/browser/screen.js
Outdated
@@ -375,7 +725,10 @@ function ScreenAdapter(options, screen_fill_buffer) | |||
|
|||
function update_scale_graphic() | |||
{ | |||
elem_set_scale(graphic_screen, scale_x * base_scale, scale_y * base_scale, false); | |||
if(!options.disable_autoscale) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I reverted this change in my cleanup, because it breaks this.set_scale
, and I was under the impression that you wanted to disable the automatic 2x scaling of small resolutions.
Later I realised, that you want v86 to not touch the width/height style at all, because you're using object-fit: contain
with width: 100%
or similar.
For now, I'm fine with merging this as-is, but I'm not 100% happy with this API, and will probably make some breaking changes in the future. In particular:
- The 2x auto-scaling should be disableable (or be moved into the caller)
- The
disable_autoscale
option in this PR should be changeable at runtime. I'm thinking of changing it tooptions.scale = 0
, so thatset_scale(0)
disables autoscale andset_scale(1)
restores it
Note that calling elem_set_scale
is important to get proper 1x resolution on screens where window.devicePixelRatio
isn't 1. Otherwise, canvas pixels don't render 1-to-1 as screen pixels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First, I never got around to mention it properly, but there is a bug in V86's autoscale. Secondly, the way it is written prevents embedders to apply their own scaling rules to canvas using CSS styles and classes.
The bug is not obvious: Enter fullscreen mode, switch to a graphical mode with small dimensions (320x200, anything that triggers autoscale), and then directly exit fullscreen mode again: The canvas stays zoomed and does not fall back into its non-fullscreen dimensions. The bug is unrelated to graphical text mode, you can test it at https://copy.sh/v86/ using my image FreeDOS-20m-demo2.img (20M), steps to reproduce:
# first boot up image and enter fullscreen
cd monkey
monkey.exe
# now press ESC to exit fullscreen
Before I added code to disable autoscale, I tried to fix this bug. Adding an event listener for fullscreenchange
event did not help, I believe it's a case for ResizeObserver which I felt overblown. Simply disabling it by opt-out seemed like a good middleground to me.
To the second point: The problem crystallizes in these two lines in screen.js. Function elem_set_scale() unconditionally rolls over whatever is in these CSS attributes like a train (the same is true about forcing display attribute block
for visibilty, there are more than a dozen other values besides block
that one could want to use here, like flex
or grid
).
In my personal opinion (no one needs to agree) this destructive forcing can easily be avoided by use of CSS classes (which can be overridden by the embedder) instead of direct write access from code to individual element's CSS style attributes for DOM objects that were not created by V86 (like for example the cursor DIV which is out of scope for embedders).
The disable_autoscale option in this PR should be changeable at runtime. I'm thinking of changing it to options.scale = 0, so that set_scale(0) disables autoscale and set_scale(1) restores it
Good idea, I'll look into it tomorrow.
Note that calling
elem_set_scale
is important to get proper 1x resolution on screens wherewindow.devicePixelRatio
isn't 1. Otherwise, canvas pixels don't render 1-to-1 as screen pixels.
I wasn't aware of this and will include it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't fully understood elem_set_scale() until now, thanks for the clarifications, except one question regarding this small section:
Lines 418 to 423 in d49cb4c
var device_pixel_ratio = window.devicePixelRatio || 1; | |
if(device_pixel_ratio % 1 !== 0) | |
{ | |
scale_x /= device_pixel_ratio; | |
scale_y /= device_pixel_ratio; | |
} |
What is the assumed range of window.devicePixelRatio here?
Anyway, I chose the most simple solution to address this in ea2119f for now, though that's still not really what you were describing. The solution is to disable all aspects of automatic scaling when options.scale is set to 0 (as before with disable_autoscale).
An application that sets options.scale to 0 has the full responsibilty for scaling, but is free in how it implements that. That's my proposal for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.devicePixelRatio
It can be any number bigger or equal to 1, including non-integers. The division looks a bit weird, but it indeed does the right thing for all devicePixelRatios.
Anyway, I chose the most simple solution to address this in ea2119f for now, though that's still not really what you were describing. The solution is to disable all aspects of automatic scaling when options.scale is set to 0 (as before with disable_autoscale).
Yes, that's what I had in mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
window.devicePixelRatio
It can be any number bigger or equal to 1, including non-integers. The division looks a bit weird, but it indeed does the right thing for all devicePixelRatios.
Some made up numbers to make my point:
- if scale is 10 and device_pixel_ratio is 2.0 or 3.0 (any positive integer), scale remains 10
- if scale is 10 and device_pixel_ratio is 2.5, scale becomes 4
If you don't mind, can you elaborate a bit on why these cases are treated so differently? What browser behaviour is being fought here?
I must admit that device_pixel_ratio and HiDPI screens are still a bit of a mystery to me.
Here's my understanding of device_pixel_ratio for you to correct: its value is a combination of the number P of physical pixels per css pixel (which should always be a positive integer, right?) and some small fractional value Z (between 0 and 1, correct?) stemming from the browser's zoom level, so device_pixel_ratio = P + Z.
And about HiDPI screens: On older "96-DPI" screens (like mine) browsers set P to 1, and on HiDPI screens to 2, 3 or 4.
I'm just trying to get my head around that.
Anyway, I chose the most simple solution to address this in ea2119f for now, though that's still not really what you were describing. The solution is to disable all aspects of automatic scaling when options.scale is set to 0 (as before with disable_autoscale).
Yes, that's what I had in mind.
Great, we'll leave it at that, then.
…e mappings Introduce support for externally defined codepage mappings wherever charmap[] is used. - added member charmap_default[] with former content of charmap[] - made charmap[] initially point to charmap_default[] - added method set_charmap() to override or fall back to charmap_default[]
options.scale defines the initial value for ScreenAdapter.scale_x and ScreenAdapter.scale_y. If undefined, options.scale defaults to 1. If options.scale is set to 0, 2x-upscale, scale_x, scale_y and HiDPI-adaption (window.devicePixelRatio) are all disabled in all three modes, they are in the responsibilty of host applications that define options.scale to be 0.
- replaced pixel-drawing with glyph-blitting - removed graphical_text_buffer - added 3 OffscreenCanvas objects for font bitmap, screen canvas and row helper - left state of canvas context ScreenAdapter.graphic_context untouched
An invisible glyph could be an ASCII Space (0x20) or Null (0x0) character, or any other empty glyph loaded into the VGA's font memory. Invisible glyphs (which are not rare) are currently drawn to the canvas exactly like regular glyphs, this patch handles that more efficiently. - added array with per-glyph visibility attribute - added invisible glyph detection in render_font_bitmap() - instead of drawing invisible glyphs in render_changed_rows(), now erase whole row buffer once per row and just skip invisible glyphs - removed one nested loop from render_font_bitmap() Characters hidden on screem caused by their blink attribute (rare) are also treated as invisible.
@chschnell This is good to merge now, do you mind if I squash? |
Yes, it's good to go. |
@chschnell Thanks for the contribution! |
You're welcome, and I'm really pleased with the outcome. |
Added alternate text mode "graphical text mode" that uses internal VGA fonts and settings in text mode with the canvas that is otherwise used in graphical mode.
Added new configuration option
screen_options
to V86 constructor, which is needed for two screen-related options:use_graphical_text
: Boolean, enable VGAScreen's graphical text mode if truedisable_autoscale
: Boolean, disable ScreenAdapter's auto-scale feature if trueVGAScreen's current behaviour is unmodified unless
use_graphical_text
is set to true, this PR is intended to be a non-breaking change.Added new bus event
screen-settings-changed
, it reports details about the current video mode settings when graphical text mode is enabled, event arguments:[false, txt_width, text_height, font_width, font_height]
[true, gfx_width, gfx_height, virtual_width, virtual_height]
Added new VGAScreen function
grab_text_content()
that returns an array of strings containing the current text screen content (in CP437).Supported VGA text mode features:
Usage example for new optional setting
screen_options
:Open issues:
This PR should resolve #1098 (more details there).