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

Generic Tileset Batching Using Vertex Buffer Abstraction #1238

Merged
merged 27 commits into from Jul 22, 2018

Conversation

Projects
None yet
3 participants
@RobertBColton
Member

RobertBColton commented May 22, 2018

Alright, this pull request is following #1283 to implement the tileset drawing as an indexed triangle list using the previous vertex and index buffer abstractions I added for users. This significantly reduces redundant code as well as brings the tileset functionality to systems where it did not previously exist, so long as those systems implement vertex and index buffers (such as Direct3D9). This partly addresses issues like #1056 and will make systems like OpenGLES and Direct3D11 easier to provide.

This redesign also makes several changes about the way the tile batching is approached. We no longer rebuild the tile model every time a tile is added or deleted, but simply set a dirty flag to update the model on the next frame. This essentially batches together several adjacent tile_add or tile_delete calls in a way that should vastly improve room loading times as I will demonstrate.

Benchmark

I prepared a benchmark that adds 1,000 tiles to a room and then draws the fps to compare the changes. Because GM8 has no facility to benchmark code, I simply ran the test before adding any calls to get_timer to query the tile_add calls. In GM8 I was only getting about 130 fps. The benchmark disables vertical synchronization and sets the room_speed to 9999. After trying GM8, I added a timer for tile_add and upped the tile_add calls to 10,000. These are the results of that benchmark.

This pull request Ding Two (With a Texture)

Now, when I did the benchmark again, for the third time in total, I used an actual texture and had added caching of the vertex format.
Download: tile_add-benchmark-textured.zip

Graphics System Tile Add Time (mu) FPS
Direct3D9 1503 4820
OpenGL1.1 1504 5070
OpenGL3.3 1504 4040

Tile Add Benchmark Textured

This pull request Ding One

The first benchmark was the first actual benchmark on the new vertex functions too, so no system actually saw a significant improvement on fps yet. What was noticeable is that the add times then became pretty constant between the different systems.
Download: tile_add-benchmark.zip

Graphics System Tile Add Time (mu) FPS
Direct3D9 1505 832
OpenGL1.1 2034 285
OpenGL3.3 1537 4058

Current Master

Graphics System Tile Add Time (mu) FPS
Direct3D9 N/A N/A
OpenGL1.1 5390472 730
OpenGL3.3 656348 4025

It's worth mentioning, that in addition to Direct3D9 not having tiles working, OpenGL3.3 drew gibberish for me on this one.
OpenGL 3.3 Tiles Benchmark Garbage

Regression Testing

To ensure that my fixes for #1199 remain intact, I ran the Tile Collision Demo from that PR on all 3 graphics systems, showing that Direct3D9 also supports it now.

Direct3D9

Direct3D9 Tile Collisions

OpenGL1.1

OpenGL1.1  Tile Collisions

OpenGL3.3

OpenGL3.3 Tile Collisions

RobertBColton added some commits May 22, 2018

@codecov

This comment has been minimized.

codecov bot commented May 22, 2018

Codecov Report

Merging #1238 into master will increase coverage by 1.06%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
+ Coverage    12.2%   13.26%   +1.06%     
==========================================
  Files         168      168              
  Lines       17413    17438      +25     
==========================================
+ Hits         2126     2314     +188     
+ Misses      15287    15124     -163
Impacted Files Coverage Δ
...system/SHELL/Graphics_Systems/General/GScolors.cpp 4.08% <ø> (ø) ⬆️
...ystem/SHELL/Graphics_Systems/General/GSsurface.cpp 0% <ø> (ø) ⬆️
...em/SHELL/Graphics_Systems/General/GSbackground.cpp 0% <ø> (ø) ⬆️
ENIGMAsystem/SHELL/Universal_System/roomsystem.h 100% <ø> (ø) ⬆️
...MAsystem/SHELL/Graphics_Systems/General/GSstdraw.h 0% <ø> (ø) ⬆️
...system/SHELL/Graphics_Systems/General/GSsprite.cpp 0% <ø> (ø) ⬆️
...system/SHELL/Graphics_Systems/General/GScurves.cpp 0.85% <ø> (ø) ⬆️
...stem/SHELL/Graphics_Systems/General/GStilestruct.h 100% <ø> (ø)
...MAsystem/SHELL/Graphics_Systems/General/GSfont.cpp 0.12% <ø> (ø) ⬆️
...system/SHELL/Graphics_Systems/General/GSstdraw.cpp 6.42% <ø> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e830ffb...8494762. Read the comment docs.

@@ -23,7 +23,6 @@
#include <math.h>
#include <stdlib.h>
using namespace std;
#define __GETR(x) ((x & 0x0000FF))

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

We have this macro about 72x in our source. When you see it can you start removing it.

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

Yes; I need to speak with you about this senior.

This comment has been minimized.

@RobertBColton

RobertBColton Jun 6, 2018

Member

Alright; that color macro is gone now because #1265 was merged and I just merged master into this: 607dfc5

@@ -303,26 +303,26 @@ bool d3d_model_load(int id, string fname)
void d3d_model_draw(int id) // overload for no additional texture or transformation call's
{
meshes[id]->Draw();
meshes[id]->Draw();

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

we need a bounds check here for debug mode perhaps overload the [] operator?

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

Right, but perhaps you should hold this thought until #1125 so this pr doesn't run astray. There's a lot of places this is missing, better to go at it in one go. I don't think I'll look into this until a next pr.

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

My only question is why you fixed the indents and didn't move the curly braces.

This comment has been minimized.

@RobertBColton

RobertBColton Jun 10, 2018

Member

Because I intend to also deduplicate the model class in the same way I am deduplicating tiles. They will both use the underlying vertex functions I believe, but at the very least this function is going in general. Since it will look like an addition at that time, I will save the formatting for then.

@@ -20,6 +20,7 @@
#include "Bridges/General/DX9Context.h"
#include "Direct3D9Headers.h"
#include "../General/GSbackground.h"
#include "../General/GStiles.h"

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

Please don't use ../ in includes it makes it a pain to move anything later

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

Correct, but it's in a million places in each system, I don't know if maybe I should just do that in a separate pr?

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

Imo just remove them when you see them. I already made huge pr to remove all the ../../ includes but you introduced a bunch again when I waasn't looking

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

I thnk im in the blame of every header file lol

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

Tbh, iirc, you did not touch graphics, because I haven't since. I know I'm responsible for a lot of these, but if I added more I probably just forgot and went with the convention the file already had. But ok, I can redo all the files I've changed here in this pr. There's no need to add the General folder to the makefile is there? I think when me and Josh set these up we must have already done that because it has no problems finding them.

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

The commit im talking about was before you made bridges but I removed literally every ../

This comment has been minimized.

@RobertBColton

RobertBColton May 23, 2018

Member

Alright, I've made the change using rectangle select for all 4 of the screen files in e58e040

for (unsigned int t = 0; t<drawing_depths[dit->second.tiles[0].depth].tilevector.size(); ++t){
enigma_user::texture_set(drawing_depths[dit->second.tiles[0].depth].tilevector[t][0]);
//d3d_model_part_draw(drawing_depths[dit->second.tiles[0].depth].tilelist, drawing_depths[dit->second.tiles[0].depth].tilevector[t][1], drawing_depths[dit->second.tiles[0].depth].tilevector[t][2]);
for (auto &t : tile_layer_metadata[dit->second.tiles[0].depth]){

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

@JoshDreamland will cry bout you using auto prolly

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

I just copied Harri's code there from OpenGL3 and changed it because he used d3d_model functions whereas polygonz original code used glCallLists (so his was easier to "generify" basically). So, yeah, I can change it, it's whatever.

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

No, auto is fine. In this case, it's pretty clear what t is.

#include "../General/GSsprite.h"
#include "../General/GSbackground.h"
#include "DX9shader.h"
#include "../General/GStiles.h"

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

more ../

This comment has been minimized.

@RobertBColton

RobertBColton May 23, 2018

Member

Alright, I replaced ../General with Graphics_Systems/General for all 4 of the include.h files in 8844907

This comment has been minimized.

@RobertBColton

RobertBColton May 23, 2018

Member

Alright, I went a step further and removed ../General from include statements occurring in sources that are already in the folder Graphics_Systems/General as well in ad7c513

Interestingly, in one or two places we had a general header including OpenGLHeaders.h when it didn't even need to, which would have meant Direct3D9 was including OpenGL... but that's gone now.

// not allowed to include mathnc.h outside of SHELLmain
namespace enigma_user {
bool point_in_rectangle(ma_scalar px, ma_scalar py, ma_scalar x1, ma_scalar y1, ma_scalar x2, ma_scalar y2);

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

don't do this. move point_in_rectangle to a header seperate mathnc. Move ma_scalar to its own header seperate mathnc if you need to.

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member
  1. Did Josh say that's ok? That's what I was originally going to do, but then, where do we draw the line at what user math functions can be used in the engine and which ones can't?
  2. ma_scalar is actually in its own header, it's in Universal_System/scalar.h with the others:

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

collision_point is this same function and it's not hidden away. I think @JoshDreamland put the mathnc rule in place to compensate for bugs in JDI and to prevent us from doing things like using enigma's sin over std::sin

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

That's what I had presumed, so not everything in mathnc.h really needs cock-blocked. I think I may hold off making a second header until a second pr, we'll see (at least now it's in 1 place instead of 4). Regardless, since point_in_rectangle is now in universal, collision_point could actually be made to use that to increase coverage. collision_point must be kept however because its logic is slightly more complicated.

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

I definitely encourage putting this in a header. Don't we have other simple collisions functions to lump into that header? E.g, line-line, triangle-triangle, or polygon-polygon?

This comment has been minimized.

@RobertBColton

RobertBColton Jun 10, 2018

Member

Yes, we do, and I can separate them into another file I suppose.

This comment has been minimized.

@RobertBColton

RobertBColton Jun 25, 2018

Member

Addressed: 885b43d

I made an exception to just include mathnc.h for now... too lazy to split it up just for one function.

#include <map>
#include <vector>
extern std::map<int,int> tile_layer_models;

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

why isn't this in a namespace?

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

I don't know. I was just trying to get it compiling first. I actually thought that since draw_tiles helper for screen_redraw uses no system-specific functions now, it could actually be moved to GStiles.cpp too and delete some more duplicate garbage. So then GStiles.h really only has to declare draw_tiles signature in namespace enigma, and not the maps/vectors, to be used by screen_redraw. That sound ok?

@@ -309,54 +309,54 @@ bool d3d_model_load(int id, string fname)
void d3d_model_part_draw(int id, int vertex_start, int vertex_count) // overload for no additional texture or transformation call's
{
meshes[id]->Draw(vertex_start, vertex_count);
meshes[id]->Draw(vertex_start, vertex_count);

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

again needs bounds check in debug mode

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

This is the part I need to discuss with you or Josh. Due to the way I reimplemented the model functions, the way GMS does, this function can actually fail because I collapse triangle strips into indexed triangle lists sometimes (depending on which is arithmetically more efficient). What I mean is that, essentially, the vertex start may sometimes not be what the user expects because I have transformed the vertices they provided into something more efficient.

What I was actually considering was adding the even lower-level vertex_* functions and rewriting the model class with those (moving the model class to Graphics_Systems/General too). The vertex functions would not do any transformation to the vertices supplied (which I believe is how GMS works too) but the model classes will. That way models are fast and efficient, and vertex buffers are very raw/lower-level but provide you more power (but with more room to shoot yourself in the foot). Basically, the vertex_* functions could let you draw in parts, but the model functions won't.

Then finally, I would rewrite the internal generified tiles to use the new vertex_* functions. This is going to be a complicated changeset, so I need to discuss it first. But right now, Direct3D9 still doesn't draw tiles because I can't sensibly add d3d_model_part_draw to that system in a way that makes sense or is reliable.

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

That has nothing to do with the fact that you're accessing meshes[id] here without checking id < meshes.size() when debugging is active. I agree it needs to happen, and it's probably worth some design discussion, but checking your bounds in a debug macro just isn't gated on that.

This comment has been minimized.

@RobertBColton

RobertBColton Jun 10, 2018

Member

Sure, by extension of my above comments, I will add the debug macro when I generify the models.

double alpha, xscale, yscale;
int color;
};
struct tile {

This comment has been minimized.

@RemoveRusky

RemoveRusky May 22, 2018

Contributor

you should probably define default values to the struct members or require a constructor incase someone forgets to set a field or a new feild is added later

This comment has been minimized.

@RobertBColton

RobertBColton May 22, 2018

Member

Sure, but we still haven't exactly decided on defaults for the protos yet either. This was Josh's doing, I'd have to refactor the whole file, and I don't see a "need" to just yet - though it can only help make it a little "safer". I may hold off on this one until another pr as well. Regardless, this is a valid point.

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

Construction for this is emitted as generated code. Adding constructors could break the C-style initializer we're using right now, so use caution.

RobertBColton added some commits Jun 6, 2018

@RobertBColton RobertBColton force-pushed the generic-tilesets-graphics branch from 1c450ba to 819219b Jun 6, 2018

@JoshDreamland

The duplication of screen_redraw is the only thing that's bothering me. I thought fundies fixed that. Was that rolled back when I rolled back his YAML changes? Or am I mistaken? This will conflict with that refactor, if it exists somewhere.

@@ -568,7 +568,7 @@ class Mesh
{
if (!GetStride()) { return; }
if (vertexbuffer == NULL || !vbobuffered) {
vbobuffered = true;
vbobuffered = true;

This comment has been minimized.

@JoshDreamland
@@ -303,26 +303,26 @@ bool d3d_model_load(int id, string fname)
void d3d_model_draw(int id) // overload for no additional texture or transformation call's
{
meshes[id]->Draw();
meshes[id]->Draw();

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

My only question is why you fixed the indents and didn't move the curly braces.

for (unsigned int t = 0; t<drawing_depths[dit->second.tiles[0].depth].tilevector.size(); ++t){
enigma_user::texture_set(drawing_depths[dit->second.tiles[0].depth].tilevector[t][0]);
//d3d_model_part_draw(drawing_depths[dit->second.tiles[0].depth].tilelist, drawing_depths[dit->second.tiles[0].depth].tilevector[t][1], drawing_depths[dit->second.tiles[0].depth].tilevector[t][2]);
for (auto &t : tile_layer_metadata[dit->second.tiles[0].depth]){

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

No, auto is fine. In this case, it's pretty clear what t is.

// not allowed to include mathnc.h outside of SHELLmain
namespace enigma_user {
bool point_in_rectangle(ma_scalar px, ma_scalar py, ma_scalar x1, ma_scalar y1, ma_scalar x2, ma_scalar y2);

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

I definitely encourage putting this in a header. Don't we have other simple collisions functions to lump into that header? E.g, line-line, triangle-triangle, or polygon-polygon?

}
}
}
void rebuild_tile_layer(int layer_depth)
{
/*

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

If this function is permanently obsolete, why comment it rather than delete it?

This comment has been minimized.

@RobertBColton

RobertBColton Jun 10, 2018

Member

Because I don't know if I should make it obsolete yet or not. I don't like the way it's designed and I had this discussion with Rusky already. Instead of rebuilding the tile model each time tile_add is called, I would like to defer building the tile model until the first screen_redraw where tiles have a "dirty" state. That way adjacent tile_add and tile_delete calls do not have as big of a performance hit.

@@ -309,54 +309,54 @@ bool d3d_model_load(int id, string fname)
void d3d_model_part_draw(int id, int vertex_start, int vertex_count) // overload for no additional texture or transformation call's
{
meshes[id]->Draw(vertex_start, vertex_count);
meshes[id]->Draw(vertex_start, vertex_count);

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

That has nothing to do with the fact that you're accessing meshes[id] here without checking id < meshes.size() when debugging is active. I agree it needs to happen, and it's probably worth some design discussion, but checking your bounds in a debug macro just isn't gated on that.

@@ -145,8 +142,9 @@ static inline int draw_tiles()
{
if (dit->second.tiles.size())
{
glCallList(drawing_depths[dit->second.tiles[0].depth].tilelist);
texture_reset();
for (auto &t : tile_layer_metadata[dit->second.tiles[0].depth]){

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

Didn't cheeseboy like just deduplicate all this? Why's this logic still in three places?

This comment has been minimized.

@RobertBColton

RobertBColton Jun 10, 2018

Member

Not that I am aware of, but screen_redraw definitely needs deduplicated. I've been communicating my desire to deduplicate screen_redraw to fundies for some time. I was intending to do it after generifying models/tiles with the vertex functions. That reduces a graphics system to almost nothing, and will make it easier for him to add GLES and do Android.

double alpha, xscale, yscale;
int color;
};
struct tile {

This comment has been minimized.

@JoshDreamland

JoshDreamland Jun 10, 2018

Member

Construction for this is emitted as generated code. Adding constructors could break the C-style initializer we're using right now, so use caution.

@RobertBColton RobertBColton force-pushed the generic-tilesets-graphics branch from a74fdf6 to bf6b4d0 Jun 17, 2018

@RobertBColton RobertBColton changed the title from Graphics System Revamp to Generic Tileset Batching Using Vertex Buffer Abstraction Jun 18, 2018

@RobertBColton RobertBColton referenced this pull request Jun 24, 2018

Merged

Generic 3D Models #1289

@RobertBColton RobertBColton force-pushed the generic-tilesets-graphics branch from 4b0ece8 to bf6b4d0 Jun 24, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Jul 7, 2018

@RobertBColton RobertBColton force-pushed the generic-tilesets-graphics branch 5 times, most recently from d90494c to 885b43d Jul 17, 2018

I addressed all of his concerns already with respect to the tile drawing test.

@RobertBColton RobertBColton merged commit d82160b into master Jul 22, 2018

4 checks passed

codecov/patch 76.92% of diff hit (target 12.2%)
Details
codecov/project 13.26% (+1.06%) compared to e830ffb
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RobertBColton RobertBColton deleted the generic-tilesets-graphics branch Jul 22, 2018

JoshDreamland added a commit that referenced this pull request Nov 10, 2018

Generic Tileset Batching Using Vertex Buffer Abstraction (#1238)
* Use vertex and index buffer abstractions to render tiles now (rendered as indexed triangle list)
* Defer building the tiles model until the first frame in which the state is "dirty"
* Cache some vertex format data for efficiency
* Use the mathnc.h header for point_in_rectangle
* Delete some unused macros, such as GL_BGR
* Cleanup includes of some general headers (fundies hates the ../ craps)
* Do not use D3DLOCK_DISCARD on static (aka "frozen") buffer lock
"This option is only valid when the resource is created with dynamic usage"
https://docs.microsoft.com/en-us/windows/desktop/direct3d9/d3dlock
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment