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

ggez-0.6 update #50

Merged
merged 16 commits into from Sep 14, 2021
Merged

ggez-0.6 update #50

merged 16 commits into from Sep 14, 2021

Conversation

PSteinhaus
Copy link
Member

@PSteinhaus PSteinhaus commented Sep 4, 2021

Ok, this PR is going to be a big one, sry.
But it's not as bad as it seems, as most of the long code is just copy-pasted examples from ggez.

I basically changed the API to conform to ggez-0.6 and then added example after example.
For each example I implemented (i.e. mostly copied from ggez) the functionality that was necessary to run it, thereby pushing good-web-game closer towards being a complete ggez implementation.

If you go through the changes you can see that I also changed some things here and there (for example removing InputHandler from KeyboardContext), but I tried not to touch any sensitive parts like the filesystem, where opinions on how to move forward may differ. So I tried to stick to things like bumping lyon, etc.

I also ran into some nasty problems, which I wasn't able to fix on my own until now.
The most prominent one being the one discussed in #40, that drawing things like text or meshes sometimes leads to a STATUS_ACCESS_VIOLATION, which can sometimes be worked around by adding even more draw calls for some reason, but sometimes can't... (see the many TODOs)

Other problems include things like color being off in the colorspace example, not being able to render to canvases sometimes and miniquad::EventHandlerFree::key_down_event always handing us repeat values of false, even when the key was obviously repeated (this last one should be easy enough to workaround, but I thought I should mention it in case this bug is still present in miniquad; maybe updating to a newer version of miniquad might fix it, haven't checked yet.

Now the question is how we want to advance. We could either keep this PR open and work on my fork, or we could merge it right away and work on it right here (meaning using the issue tracker of this repo instead of my fork). I'd prefer the latter option, but I could of course understand if you didn't want to merge this PR before being 100% content with it.

EDIT: I just saw that #49 also features some fixes, including some that this PR doesn't, so I guess it might be better to merge that one first, perhaps.

added some of the ggez-0.6 examples;
expanded functionality (implemented more of ggez-0.6)
@PSteinhaus
Copy link
Member Author

PSteinhaus commented Sep 4, 2021

Also, I didn't use the latest version of miniquad for creating this PR, but instead sticked to the one used currently.
This might be something to change before merging the PR, maybe.

@not-fl3
Copy link
Collaborator

not-fl3 commented Sep 4, 2021

Wow, very impressive work!

The most prominent one being the one discussed in #40, that drawing things like text or meshes sometimes leads to a STATUS_ACCESS_VIOLATION

We had similar problem in macroquad, and doing this(moving delete after SwapBuffers) helped:
#40 (comment).

maybe updating to a newer version of miniquad might fix it, haven't checked yet.

Miniquad used here is really old (0.3-alpha is a recommended version to use, it moved quite a lot from 0.2).

Now the question is how we want to advance. We could either keep this PR open and work on my fork, or we could merge it right away and work on it right here (meaning using the issue tracker of this repo instead of my fork). I'd prefer the latter option, but I could of course understand if you didn't want to merge this PR before being 100% content with it.

I am not actively maintaining good-web-game anymore, so, if you want, I can give you admin rights here on this repo or even transfer you the ownership.

Cargo.toml Outdated
@@ -14,10 +14,16 @@ cgmath = { version = "0.17", features = ["mint"] }
miniquad_text_rusttype = { git = "https://github.com/not-fl3/miniquad_text_rusttype" }
miniquad = "=0.2.55"
Copy link
Collaborator

Choose a reason for hiding this comment

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

miniquad 0.2 is seriously outdated, there are lots, lots of bugfixes in 0.3.0-alpha.37

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I'll try to bump it to the latest version of miniquad then ;)

@PSteinhaus
Copy link
Member Author

Wow, very impressive work!

Thank you ^^ looking at the line count I feel somewhat impressed myself, but it's mostly copies of the wonderful code made by all the wonderful contributors of ggez, really.

The most prominent one being the one discussed in #40, that drawing things like text or meshes sometimes leads to a STATUS_ACCESS_VIOLATION

We had similar problem in macroquad, and doing this(moving delete after SwapBuffers) helped:
#40 (comment).

Hmm, interesting. I haven't investigated deeply, but I couldn't find any references to glDeleteBuffers in good-web-game, or the surrounding libraries, but I assume it happens as part of ctx.quad_ctx.end_render_pass()?

Looking into this I just realized present() is nothing more than a dummy in good-web-game 😅 I really gotta look into miniquad to find out more about how it handles all of this.

Now the question is how we want to advance. We could either keep this PR open and work on my fork, or we could merge it right away and work on it right here (meaning using the issue tracker of this repo instead of my fork). I'd prefer the latter option, but I could of course understand if you didn't want to merge this PR before being 100% content with it.

I am not actively maintaining good-web-game anymore, so, if you want, I can give you admin rights here on this repo or even transfer you the ownership.

Wow! Thank you for that offer! I'd gladly accept admin rights. It'd make working on this much easier, thank you.
There are different things where I'd want to, at least, ask you for your opinion, both for reasons of experience and original ownership, still.

@not-fl3
Copy link
Collaborator

not-fl3 commented Sep 4, 2021

Hmm, interesting. I haven't investigated deeply, but I couldn't find any references to glDeleteBuffers in good-web-game, or the surrounding libraries, but I assume it happens as part of ctx.quad_ctx.end_render_pass()?

IIRC the problem occurs when you

  • create buffer
  • draw buffer
  • delete buffer
  • do a commit_frame or just end the draw handler

ggez do buffers reallocation all the time, so it happens quite often. But I did not really research whats going on after the error got fixed in macroquad.

Wow! Thank you for that offer! I'd gladly accept admin rights. It'd make working on this much easier, thank you.
There are different things where I'd want to, at least, ask you for your opinion, both for reasons of experience and original ownership, still.

Done, you are an admin (or will be after following this link: https://github.com/not-fl3/good-web-game/invitations
) :)

@PSteinhaus
Copy link
Member Author

PSteinhaus commented Sep 5, 2021

Ok, updating to miniquad 0.3.0-alpha.37 alone didn't fix anything AFAIK, too bad.
In fact, the astroblasto example is now somewhat broken (but that's probably just due to outdated RNGs and should be easy to fix, hopefully).

IIRC the problem occurs when you

* create buffer

* draw buffer

* delete buffer

* do a `commit_frame` or just end the `draw` handler

ggez do buffers reallocation all the time, so it happens quite often. But I did not really research whats going on after the error got fixed in macroquad.

Hmm, is there a thread on this in your issue tracker? (searched a bit already, but couldn't find one)
Where/when does miniquad swap its buffers?
I have a few questions regarding miniquad in general, I guess I'll take a look into the discord server.

Done, you are an admin (or will be after following this link: https://github.com/not-fl3/good-web-game/invitations
) :)

Thx! ❤️

@not-fl3
Copy link
Collaborator

not-fl3 commented Sep 6, 2021

Hmm, is there a thread on this in your issue tracker? (searched a bit already, but couldn't find one)

#40 (comment)

This is the minimum repro and an explanation on why it crashes

Where/when does miniquad swap its buffers?

It depends on the platform. On WebGL there is no swapbuffer whatsoever: https://stackoverflow.com/questions/41018445/where-is-the-swapbuffer-opengl-call-in-webgl/41045668#41045668

On windows, where the crash persists, miniquad calls glSwapBuffer right after a frame callback

@PSteinhaus
Copy link
Member Author

Hmm, is there a thread on this in your issue tracker? (searched a bit already, but couldn't find one)

#40 (comment)

This is the minimum repro and an explanation on why it crashes

Of course. Sorry, I should have been more precise. I meant: is there a thread on the macroquad issue tracker that I missed?
I'm not that experienced in OpenGL yet and it would be interesting for me to see how you fixed it in macroquad.
I guess I should take a closer look at macroquad in general.

src/lib.rs Outdated
@@ -136,18 +139,22 @@ impl<E: std::error::Error> miniquad::EventHandlerFree for EventHandlerWrapper<E>
}
}

pub fn start<F, E>(conf: conf::Conf, f: F) -> GameResult
pub fn start<F, E>(quad_conf: miniquad::conf::Conf, conf: conf::Conf, f: F) -> GameResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not recommend using miniquad's Conf in public API.

Cache, for example, is gwg-only thing and should be removed from miniquad's Conf.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(cache in miniquad is a legacy from times when miniquad was not able to load files by url)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I didn't know that. Thx for explaining.
Then I'll put cache back into the good-web-game Conf. It fits better anyway.
I'll probably just add all the miniquad fields to it as well and then simply create a miniquad Conf to hand over when starting the application.

added some missing graphics functions;
updated tar file;
made canvas flipping slightly more efficient;
refactored setting blend modes a little;
started work on a MeshBatch implementation;
added appropriate alpha blending to all pipelines;
cleaned up some examples;
enabled filters on textures used in meshes;
added meshbatch pipeline (though it's not used yet);
@PSteinhaus
Copy link
Member Author

Ok, I'm happy with how this turned out :)
All the important stuff from ggez is now included, with one exception: using your own shaders.
I'll not include this feature in this PR, as I'm probably not the right one to write it. I leave it to someone more experienced in the field (i.e. shaders in miniquad).

This was referenced Sep 13, 2021
added default filter functions;
updated documentation;
added functions for grabbing/hiding the cursor and setting cursor type;
removed Loading from conf, as it currently doesn't to anything;
@PSteinhaus PSteinhaus merged commit ce64fd4 into ggez:master Sep 14, 2021
@erlend-sh
Copy link

🎉 👏

About page should probably be updated:

[DEPRECATED] A minimal 2d game framework for wasm.

@PSteinhaus
Copy link
Member Author

🎉 👏

About page should probably be updated:

[DEPRECATED] A minimal 2d game framework for wasm.

Yup, couldn't do that before getting ownership though ;)

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.

None yet

3 participants