fixes(#55): add MemoryHints struct that mirrors wgpu's and allows for additional egor buffer control

[jonatino]

This should allow for more than enough control over the biggest configurable buffers in both egor and wgpu. This more than makes egor viable for my use case of retro low-res, low memory games.

Sample:

use std::usize;

use egor::app::App;
use egor::app::FrameContext;
use egor::app::MemoryHints;
use egor::input::KeyCode;
use egor::math::Vec2;
use egor::math::vec2;
use egor::render::Color;

fn main() {
    let mut position = Vec2::ZERO;

    let hint = MemoryHints::MemoryUsage;

    App::new()
        .memory_hints(hint)
        .title("Egor Stateful Rectangle")
        .run(
            move |FrameContext {
                      gfx, input, timer, ..
                  }| {
                let dx = input.key_held(KeyCode::ArrowRight) as i8
                    - input.key_held(KeyCode::ArrowLeft) as i8;
                let dy = input.key_held(KeyCode::ArrowDown) as i8
                    - input.key_held(KeyCode::ArrowUp) as i8;

                position += vec2(dx as f32, dy as f32) * 100.0 * timer.delta;

                gfx.rect().at(position).color(Color::RED);
            },
        )
}

Ran on windows 11 with nvidia 4090:

MemoryHints::Performance

#default 115mb
#gles 146mb
#angle 115mb
#vulkan 233mb

MemoryHints::Memory

#default 50mb
#gles 81mb
#angle 50mb
#vulkan 96mb

[wick3dr0se]

Great idea to factor in the buffer sizes as well

I think passing MemoryHints into OffscreenTarget isn't really necessary and kind of messes up that API a bit because ideally the memory profile would be set only initially when App is created. And render targets stay dumb. So we should be able to simplify that some. Also thinking naming it MemoryProfile over mirroring wgpus naming would be nice

At work now but once I get time after, I will look into this and provide better feedback/changes

[jonatino]

@wick3dr0se yeah I didnt really like the render target stuff either but couldnt find a clean way to pass in values that might differ from App. I would like to keep being able to configure those buffers as well if possible because I use quite a few off screen targets some with varying complexity.

Could perhaps just introduce a new method instead eg:

    pub fn create_offscreen_target_with_memory_hints(
        &self,
        width: u32,
        height: u32,
        format: TextureFormat,
        memory_hints: MemoryHints,
    ) -> OffscreenTarget 

That way we wont change the signature of the old one. What do you think?

[wick3dr0se]

@jonatino That's a good point (for per-target buffer sizing) but targets don't own geometry buffers, they just exist transiently inside render_offscreen() as PrimitiveBatch. So I think the cleanest change would be a variant on Graphics like:

fn render_offscreen_with_profile(
    &mut self,
    target: &mut OffscreenTarget,
    profile: MemoryProfile,
    render_fn: impl FnMut(&mut Graphics),
)

This would clean it up quite a bit, dropping the changes to target.rs, Renderer and the triangle example. Making Renderer::new() simply just take memory_hints field and apply it as it does now. No need for the memory_hints() method on Renderer since OffscreenTargets won't use that anymore either

Also just preference but we could name it MemoryProfile to be a bit more clear since we're more than just hinting the GPU driver like wgpu does, with the buffers

[wick3dr0se]

@jonatino I tinkered with it on a new branch to see what it would look like. After messing with it, I think it makes more sense to just expose MemoryHints straight from wgpu and let users pass it into Renderer::new()/App::new().memory_hints(). Since the renderer already exposed GeometryBatch::new() letting users pass in max vertices/indices, that part was handled there. So what I did was just make a separate builder method on App for setting them (batch_limits()) and made a new method called render_offscreen_with_limits() on Graphics. I haven't fully tested it yet but if you get a chance to try look at it, let me know what you think!

db199f1 (pr-78-rev branch)

[jonatino]

@wick3dr0se yeah your changes are much much cleaner. I'm still a little new to rust so forgive me 😂

Would you like me to merge your changes into my branch?

[wick3dr0se]

@jonatino Sweet!! haha no worries dude! There is a lot of layers to egor with the separation

I can likely push it straight to your branch in your repo, which will just update the PR too. I'll see if I can do that and get it merged. Should be less work if it works lol

[wick3dr0se]

@jonatino Sorry for the delay, been a bit tied up

I just went to try this finally and it seems I either don't have permission to push to your fork or maybe main branch protection is on. If you're still open to merging the changes into your branch, seems that might be easiest. Otherwise maybe you can change permissions and I can try it out. Either way works for me

[jonatino]

@wick3dr0se done

[wick3dr0se]

@jonatino a few broken doc links keeping CI from passing. Just need to change render_offscreen to Self::render_offscreen, GeometryBatch links to use full path from egor_render::batch::GeometryBatch and wgpu::MemoryHints just needs to be MemoryHints since we are importing it from egor_renders re-export instead (edit: put in suggestions)

Perms make me create a new branch but I think it's just due to the PR being made from a main branch. If you could fix these, would be much appreciate, otherwise it's ready for merging!

[wick3dr0se]

@jonatino Thanks for your work! Merged!!!