Skip to content
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

Version 0.4 Full Release Tracking Issue #552

Open
37 of 43 tasks
ryanisaacg opened this issue Jan 5, 2020 · 30 comments
Open
37 of 43 tasks

Version 0.4 Full Release Tracking Issue #552

ryanisaacg opened this issue Jan 5, 2020 · 30 comments
Labels
change-api-breaking A backwards-incompatible change to the API help wanted: discussion Feel free to contribute your thoughts

Comments

@ryanisaacg
Copy link
Owner

ryanisaacg commented Jan 5, 2020

During Alpha

Features that have yet to be (re-)implemented, and are planned:

Features that are yet to be (re-)implemented, and are a maybe:

Documentation:

Feedback required:

  • Should the Scalar API be kept? (e.g. keep paying the compile-time process for generics to be able to write terse things like Rectangle::new((x, y), (width, height)) or require explicitness)
  • Should geometry be split into a separate crate to allow use outside Quicksilver?
  • Should image formats other than PNG be supported out-of-the-box? They're not widely used, can cause issues (Loading a jpg image causes a panic in WASM #508), and cause binary bloat.
  • How should key repeats be handled? (See Key presses get repeated #523)

Bugs / tickets:

Investigate:

  • Canvas high-dpi
  • Are all the re-export paths sensible or do some require referencing external crates?

API reworks from alpha experience:

When Alpha ends:

During Beta

Future Minor Release

Future Major Release

  • Audio support
  • Migrate default / recommended web backend to web-sys
@ryanisaacg ryanisaacg added change-api-breaking A backwards-incompatible change to the API enhance-version-bump help wanted: discussion Feel free to contribute your thoughts labels Jan 5, 2020
@lenscas
Copy link
Contributor

lenscas commented Jan 5, 2020

Should image formats other than PNG be supported out-of-the-box? They're not widely used, can cause issues (#508), and cause binary bloat.

Personally I can see someone writing code that on the web it first loads jpg versions of (some of) the textures to get the loading done quicker and after that load the better png versions on the background. Especially combined with a service worker to get the png images in cache for next time.

However, according to the linked issue this isn't an option anyway as jpg's crash on the web. Add to this that I see no benefit for them on native and my vote is a clear remove. At least for the time being until it can work again on the web.

As for svg's (Which I assume is the other format being talked about). I have no problem with their removal as I don't use them myself. I don't think they are great for textures leaving their use just for UI elements. I also don't think most other engines have support for svg anyway (at the very least unity doesn't) so not having support for svg's isn't even that surprising.

So, my vote on svg support: It is nice to have but I won't care if it gets removed.

@ryanisaacg
Copy link
Owner Author

Svgs aren't actually the other formats I was mentioning (though they are vaguely supported through lyon, once that is re-implemented). The image crate includes tiff, gif, jpeg, bmp, ico, and png. Of those, I think png is used the most, but every crate that pulls in Quicksilver takes a compile time and binary size hit to support the others.

@MaulingMonkey
Copy link
Contributor

Huzzah, progress! Some minor notes from an upgrade attempt, since I'm not sure if these are just tracking missing API bits, or if they include default implementation stuff:

Features

  • Blending support

The default blending mode in 0.4.0-alpha0 appears to ignore the image alpha channel now on both desktop (transparent areas are black) and web (transparent areas are white!).

  • Resize handling

Settings already exposes resize, but the GL viewport is neither automatically set, nor settable through quicksilver, making it impossible to render to the expanded window size.

Feedback

  • Should the Scalar API be kept? (e.g. keep paying the compile-time process for generics to be able to write terse things like Rectangle::new((x, y), (width, height)) or require explicitness)

No strong preference either way. The case where I get more annoyed is when encountering things like Window::size() or Image::size() returning f32 vectors that I want to convert back to integers, triggering paranoia about fractional pixels, negative dimensions, and NaNs.

  • Should geometry be split into a separate crate to allow use outside Quicksilver?

Might be better to try and extend an existing math crate like nalgebra to contain the types?

I think it's fine for the default feature set to not support PNGs. Just have a decent error message for devs when you try to load .jpeg s anyways.

This is a whole rabbit hole, be aware that fully solving the problem perfectly is nontrivial to impossible. Having a basic "best effort" BTreeSet<Key> somewhere that's set on keydown - and cleared on keyup and window blur - would be reasonable and nice. Forwarding any OS-provided "repeat" bits might also be useful (maybe as a Modifiers bit?)

Some rabbit hole details:

OS event queues may conflate left/right modifiers like control, shift, alt, etc. in a way that makes perfect key state tracking annoying, impossible, or expensive.

XB1 chatpads like https://user-images.githubusercontent.com/33192525/63055071-f323e380-be99-11e9-81c2-9fe66b51310c.jpg send keyup despite holding buttons, and then re-spam keydown / keyup pairs on repeats.

IE11 also doesn't set the input event repeat field on repeat events, but IE11 doesn't natively support WASM so we probably don't care, even if some crazy person might try to get it working via polyfill.

@lenscas
Copy link
Contributor

lenscas commented Jan 8, 2020

Svgs aren't actually the other formats I was mentioning (though they are vaguely supported through lyon, once that is re-implemented). The image crate includes tiff, gif, jpeg, bmp, ico, and png. Of those, I think png is used the most, but every crate that pulls in Quicksilver takes a compile time and binary size hit to support the others.

bmp is (if I remember correctly) uncompressed and thus a bad choice for anything web related. That alone is in my opinion enough reason not to support it.

I don't think anyone will try tiff and ico files, because at least to me they seem like weird choices for sprites. So, I think they are save to go.

I care little about what happens to gif support.

Png's should stay

Obviously, nice error messages along with documentation on what is supported is always appreciated :)

@elsassph
Copy link

elsassph commented Jan 8, 2020

JPEG would have been nice for web target.

@ryanisaacg
Copy link
Owner Author

It seems like jpeg support is desired, so it stays! See #558. As a nice side effect, rayon is no longer included in the build.

@MaulingMonkey

The default blending mode in 0.4.0-alpha0 appears to ignore the image alpha channel now on both desktop (transparent areas are black) and web (transparent areas are white!).

That was a bug, my bad. Fixed in #560

Settings already exposes resize, but the GL viewport is neither automatically set, nor settable through quicksilver, making it impossible to render to the expanded window size.

Yup, that's part of what remains to be done.

No strong preference either way. The case where I get more annoyed is when encountering things like Window::size() or Image::size() returning f32 vectors that I want to convert back to integers, triggering paranoia about fractional pixels, negative dimensions, and NaNs.

Good point; things that can be ints, probably should be.

Might be better to try and extend an existing math crate like nalgebra to contain the types?

With the exception of maybe vek, most math crates have some minor or major problem that puts me off using them.

keyboard event rabbit hole

Thanks for that info, which is a good look into how horrifying platform APIs are. I think I agree, that a simple best-effort set of currently-down keys (and maybe a separate set for mouse buttons, gamepad buttons, etc

@elsassph
Copy link

What's the story for framerate control? Will there be a helper in the lib?

@ryanisaacg
Copy link
Owner Author

ryanisaacg commented Jan 13, 2020

Yup, the checkmark for that is "Helpers for planning when to update / draw"

@paulirotta
Copy link
Contributor

Would you be open to having a "canvas size matches screen/web-window size" feature for full screen?

Something along the lines of:

canvas.set_width(canvas.offset_width() as u32);
canvas.set_height(canvas.offset_height() as u32);

( https://github.com/koute/stdweb/blob/master/examples/canvas/src/main.rs )

@elsassph
Copy link

Related to @paulirotta 's question: I noticed the canvas isn't high dpi.

@ryanisaacg
Copy link
Owner Author

@paulirotta Hm, why match the canvas to the size of the window rather than native fullscreen?

@elsassph If the canvas isn't high-dpi, that could be a winit bug. I'll add investigating that as a task for this issue.

@paulirotta
Copy link
Contributor

@ryanisaacg Native full screen sounds great!

@lenscas
Copy link
Contributor

lenscas commented Jan 22, 2020

Personally, I wouldn't mind to also have an option to set the width and height of the canvas to the size of the browser window. I can defiantly see people (Even myself) wanting to play a game in a screen as big as possible without hiding the rest of the browser.

On native this option should just be the same as maximizing a program.

@lenscas
Copy link
Contributor

lenscas commented Feb 24, 2020

I have a question regarding font/text rendering.

I have a sudden urge to try and get a text input to work in Mergui. This will probably be done by rendering every character inside of it separately, instead of making 1 big image out of the text.

Do you foresee many changes between the current system and the new one? And if so, are there things I can already plan for?

Thanks in advance and also, thanks for making quicksilver :)

@ryanisaacg ryanisaacg mentioned this issue Feb 24, 2020
6 tasks
@ryanisaacg
Copy link
Owner Author

@lenscas I've been working on font support locally, which I've just pushed up as #581. The new font system essentially bears no resemblance to the old one, so instead of trying to explain the differences I figured showing them would be better.

@lenscas
Copy link
Contributor

lenscas commented Feb 28, 2020

Thanks for the previous answer, it gave me everything I needed to know to start porting mergui :)

However, a few more questions

I saw that there is no function to draw with an Z order. Is that function coming back/am I blind? I am also guessing that this means that draw order now works correctly? ( 🎉 )

If it is coming back, how would a draw with a z value interact with one that doesn't? Specifically in the case where the draw without a z value comes after the one with one set? Its fine if you don't know this one, it is more to manage my own time :)

Thanks in advance for answering :)

@ryanisaacg
Copy link
Owner Author

Z ordering is dead, and draw order Just Works correctly.

@lenscas
Copy link
Contributor

lenscas commented Apr 21, 2020

In the case where you draw to a texture after having made some draw calls that are supposed to go to the window. Is it required to flush first before calling Graphics::fit_to_surface, or is there another way to get the desired result?

//these 2 are supposed to be rendered to the window
gfx.clear(Color::BLACK);
gfx.fill_rect(
    &Rectangle::new(Vector::new(350.0, 100.0), Vector::new(100.0, 100.0)),
    Color::BLUE,
);

gfx.flush(None)?; //This one seems to be needed

//the calls from this point on need to go to the surface
let mut surface = Surface::new(
    &gfx,
    Image::from_raw(&gfx, None, 512, 512, PixelFormat::RGBA)?,
)?;
// Set the render area to the surface size
gfx.fit_to_surface(&surface)?;
gfx.clear(Color::WHITE);

@ryanisaacg
Copy link
Owner Author

This is a failure of the API to communicate what's actually happening. gfx.fit_to_surface doesn't change the render target; it changes what the viewport is. You need to call gfx.flush(None)? to send the previous draw commands to the window, and presumably later you'll call gfx.flush(Some(&surface))? to send some draw commands to that surface.

@lenscas
Copy link
Contributor

lenscas commented Apr 21, 2020

Ok, then I'm not doing anything wrong :)

Maybe gfx.fit_to_surface should do the call to gfx.flush for you? And if there is a significant cost to flushing and a method that doesn't do it? Or perhaps an extra enum argument to tell it to either flush or not?

I can probably make a pull request for this this weekend.

@lenscas
Copy link
Contributor

lenscas commented Apr 23, 2020

I have a weird problem when rendering to a texture. The generated image seems to always be black. However,if I copy the code to to another project it works as expected. The rest of the window gets rendered correctly. So, I'm not even sure if I have other drawing code that causes the problem :(

The code in question

gfx.flush(None)?;
let mut surface = Surface::new(
    gfx,
    Image::from_raw(gfx, None, 512, 512, PixelFormat::RGBA)?,
)?;
gfx.fit_to_surface(&surface)?;
gfx.clear(Color::BLUE);
gfx.fill_rect(&Rectangle::new((0, 0), (10, 10)), Color::GREEN);
gfx.flush(Some(&surface))?;
//gfx.clear(Color::RED);
gfx.fit_to_window(&window);
let image = surface
    .detach()
    .ok_or(quicksilver::QuicksilverError::SurfaceImageError)?;
gfx.draw_image(&image, Rectangle::new((10, 10), (512, 512)));

In case it helps, I commited how far I got, the code can be viewed at :
https://github.com/lenscas/mergui/blob/21ea53807a9097fdd1638ec01c265475a29692ca/src/widgets/input.rs#L140

I use the all example to test it

Sorry in advance of the state of that file. I was busy in redoing how it worked to make use of rendering to texture and I tried many things after that to try and discover why it didn't work.

@ryanisaacg
Copy link
Owner Author

It looks like everything you're doing is aboveboard, can you open this as an issue? I think it's probably a Quicksilver bug.

@lenscas
Copy link
Contributor

lenscas commented May 3, 2020

I have a question about Graphics::into_raw_context(self) and what it means for libraries that need Graphics to draw things.

Right now, a library that needs to draw things can not be used by people who want to use custom shaders because there is no way to get the Graphics object back. A way to reduce this problem is to introduce a new trait that is implemented by Graphics and contains the same or a subset of the draw calls currently implemented by Graphics.

This means that libraries can be generic over this trait if the maintainer wants to support users with custom shaders, and developers who write custom shaders have a single trait to implement to be able to use these libraries.

My questions are:

  • Would a trait like this make sense to exist at all.
  • Would a trait like this make sense to be included inside quicksilver? Or do you think every library that needs to draw using quicksilver is better off making their own trait(s) for this purpose instead?

Thanks in advance for answering these 2 questions.

@ryanisaacg
Copy link
Owner Author

@lenscas When using custom shaders, it's hard to maintain the soundness of the Graphics API. golem relies on the user upholding the contract of the prepare_draw. The reason you lose access to the Graphics API is to ensure the safety of flush.

It may be better, rather than trying to replicate the entire Graphics interface through a trait, to have a trait that just incorporates what you need.

@lenscas
Copy link
Contributor

lenscas commented May 4, 2020

Ok, this makes sense. Thanks for the answer :)

@paulirotta
Copy link
Contributor

Rasperry Pi 4 raspbian, example 02_image throws a (kudos) clear whine:

ERROR [golem::shader] Failed to compile vertex shader: 0:1(10): error: GLSL 1.50 is not supported. Supported versions are: 1.10, 1.20, 1.00 ES, and 3.00 ES

@ryanisaacg
Copy link
Owner Author

@paulirotta I opened an issue at ryanisaacg/golem#28 to investigate / fix this.

@lenscas
Copy link
Contributor

lenscas commented May 8, 2020

I decided to make a simple animation system that can either be its own library or be merged with Quicksilver (Whatever you prefer)

However when writing it I noticed that there doesn't seem to be a way to rotate a single image. I saw Graphics::set_transform() however that doesn't seem to work for this as simply setting the rotation causes my image to rotate around a certain point, rather than it spinning at its center.

I only needed it for an example so it isn't a huge deal right now as I can just change the color or something for the time being but it did kind of surprise me.

Note: it may very well just be me overlooking something. In case it is, the code I used is equivalent to

let step_size: f32 = 5.;
let amount_of_steps = (360. / step_size).ceil() as usize;
let step = tick % amount_of_steps;
let rotation = step as f32 * step_size;
let rotation = dbg!(rotation);
gfx.set_transform(Transform::rotate(rotation));
gfx.draw_image(state, location);
gfx.set_transform(Transform::IDENTITY);

@ryanisaacg
Copy link
Owner Author

@lenscas Graphics::set_transform sets a transformation matrix; if you want to spin an image at its center, you have to offset the transform to the center of the image, like:

let origin = location.center();
gfx.set_transform(Transform::translate(origin) * Transform::rotate(rotation) * Transform::translate(-origin));
gfx.draw_image(state, location);
gfx.set_transform(Transform::IDENTITY);

@nzsg
Copy link

nzsg commented May 13, 2020

I’ve been following this with interest and just wanted to say thanks for making and improving Quicksilver!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change-api-breaking A backwards-incompatible change to the API help wanted: discussion Feel free to contribute your thoughts
Projects
None yet
Development

No branches or pull requests

6 participants