Skip to content

Conversation

@rofrol
Copy link
Contributor

@rofrol rofrol commented Jan 14, 2020

Sorry for this low quality PR. I am just learning all this stuff.

In the wip commit I tried to update deps to winit 0.20.0. It succeeded in next commit but now I don't see any code displayed in the editor. Just black window.

Copy link
Owner

@cbrewster cbrewster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @rofrol! Thanks for contributing 😄

Just a heads up, this is more of a toy project at the moment, but I plan to keep working on it and am happy to accept contributions. Also the editor is currently in a not-really-functioning state, I need to finish up some work w.r.t. text selections.

Everything looks pretty good, thanks for updating winit. I've left some review comments if you have time to update a few things. As for the issue with the black screen, I'll see if I can reproduce this locally.

// TODO: Dynamically load fonts or something?
let inconsolata: &[u8] =
include_bytes!("/Users/connor/Library/Fonts/InconsolataGo-Regular.ttf");
let inconsolata: &[u8] = include_bytes!("/usr/share/fonts/truetype/ubuntu/UbuntuMono-R.ttf");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably just ship a font like Inconsolata in the repo and have a relative path here. (we would just need to make sure to add the font's license to the repo as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it'd be great to use something like font-kit for this!

..
} => {
if input.virtual_keycode == Some(VirtualKeyCode::LWin) {
if input.virtual_keycode == Some(VirtualKeyCode::LControl) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this as LWin as on macOS quit is Command-Q and not Ctrl-Q (Command and LWin are the same thing I believe). We should probably add some platform checks to make sure each platform get their correct keybindings.

@cbrewster
Copy link
Owner

I just tried locally, the patch appears to work fine although the resizing needs to be fixed (see my review comment about that). What platform are you running on?

Also the delete/backspace behavior seems a bit odd, backspace should delete one character on the left of the cursor and delete should delete a char on the right side of the cursor. When I hit backspace is performs a delete instead and hitting delete does nothing. This could be another case of weird cross-platform things?

Input handling is very basic at the moment and will need to become a bit more robust in the future to better handle different keybindings across platforms.

@rofrol
Copy link
Contributor Author

rofrol commented Jan 14, 2020

I am running Ubuntu 19.10 x86_64

@cbrewster
Copy link
Owner

Hmm okay, I can probably try this on Linux soonish, but maybe not today. Do you end up with any Vulkan validation errors when running?

@rofrol
Copy link
Contributor Author

rofrol commented Jan 14, 2020

I still see black window :/

@rofrol
Copy link
Contributor Author

rofrol commented Jan 14, 2020

Do you end up with any Vulkan validation errors when running?

No

@cbrewster
Copy link
Owner

Did it work with the old winit version?

@rofrol
Copy link
Contributor Author

rofrol commented Jan 14, 2020

yes

@rofrol
Copy link
Contributor Author

rofrol commented Jan 14, 2020

I have converted locally hello-triangle from wgpu-rs to winit 0.20 and it works

@cbrewster
Copy link
Owner

I'll look into the issue with the blank window soon. Feel free to dig into it if you'd like.
This PR looks good otherwise. Font-kit support should be added separately I think.

Thanks again!

@cbrewster cbrewster merged commit 6480b97 into cbrewster:master Jan 14, 2020
@cbrewster cbrewster mentioned this pull request Jan 14, 2020
@rofrol rofrol deleted the update-deps branch January 14, 2020 21:43
@rofrol
Copy link
Contributor Author

rofrol commented Jan 15, 2020

I run it on Windows and it works 😭

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants