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 3D Models #1289

Merged
merged 38 commits into from Aug 15, 2018

Conversation

Projects
None yet
3 participants
@RobertBColton
Member

RobertBColton commented Jun 24, 2018

This pull request attempts to leverage the new generic vertex and index buffer abstractions to generically implement a new 3D model class to replace the old duplicate ones. This pull request continues alongside #1238 and will also make newer and alternative graphics systems easier to support, such as OpenGL ES or Direct3D11, without sacrificing performance, expressiveness, or compatibility as I will demonstrate in the proceeding benchmarks.

Noteworthy changes made

  • Every old model class removed and deleted from git
  • A new generic model class was created to replace the deleted ones
  • Changed d3d_model_create to return an int like other resource functions for consistency
  • Removed d3d_model_part_draw functions because their name is inconsistent (e.g, should be draw_part like draw_background_part) and because they are not clear enough for a model class which optimizes and combines primitives together, d3d_model_draw_primitive with an index to the primitive would be a better function. We also no longer use these functions anywhere, tiles use the vertex and index buffer abstractions directly now which offer range arguments in their submit functions.
  • Added a new function d3d_model_vertex_data that allows one to specify the model's vertex data in a single function call, but requires a vertex format be provided.
  • Updated GSmodel.h with comments specifying the preconditions for each group of model specification functions
  • Renamed d3d_model_add_* functions to remove the _add_ part of their name to match the original d3d_model_vertex function from GM for consistency.
  • Added the following new vertex functions:
    • vertex_format_exists(): This non-id version of the function will tell us if we are between vertex_format_begin and vertex_format_end calls. In other words, it tells us if we are still defining a new vertex format.
    • vertex_format_get_stride(): Non-id version returns the stride of the vertex format currently being specified.
    • vertex_format_get_hash(): Non-id version returns the hash of the vertex format currently being specified.
    • vertex_format_get_stride(id): Returns the stride of the vertex format with the given id. This is basically the number of vertex attributes, it is not the size of a vertex in bytes.
    • vertex_format_get_hash(id): Returns a hash of the vertex format and its type/usage pairs.
    • vertex_set_format(buffer,format): Allows one to change the vertex format of a vertex buffer, which essentially allows the vertex buffer to hold the same data for differently formatted primitives.
  • Made vertex_format_end set the enigma::vertexFormat pointer to 0 so there can be no further accidental mutation of it once it is moved to the vector and assigned a real id for the user.
  • Made vertex_get_number, vertex_get_size, and their index buffer equivalents return the correct value when in the middle of vertex_begin and vertex_end which previously they would not. This required marking the vertex and index buffers dirty in vertex_begin instead of vertex_end so that the dirty flag could be used to determine where to grab the number of vertices/indices from.
  • Added a non-fatal error message for vertex_data that shows in debug mode if the user calls the function but the vertex buffer does not have a valid vertex format, since one is required for the function to correctly decompose the arguments.
  • Removed all d3d_model_has_* functions because we aren't using them anywhere and we don't need them.
  • Changed graphics_reset_client_state to unbind the vertex and index buffers (bind the target to 0) in OpenGL1 because it will screw up primitive rendering that uses glVertexPointer with a client-side array. This caused issues in Wild Racing GMK because d3d_draw_floor was not being drawn and I could not figure out why.
  • Removed d3d_model_get_stride and d3d_model_format as well as calls to them in the context managers in Bridges because the function no longer makes sense. The model class in this pr has a format for each primitive, hence the entire model doesn't have a consistent stride or format.
  • Removed d3d_model_index because the new model class doesn't support indexing yet.
  • Added caching of both vertex formats and vertex format peers. The peers are non-trivial to construct and we also don't want the user being wasteful with the former either. Caching both independently gave separate performance boosts, so both are necessary. Because of this change, vertex_format_end may now return the same id multiple times and never actually return a unique id if the hashes of the vertex format specified are always logically equivalent to another format.
  • Fixed a typo in d3d_model_set_rotation_x for D3D9, it should have had gs_scalar not double like its signature in the general header.
  • Added a dynamic flag to vertex_freeze so the user can indicate such usage. By default I set the parameter to false, because freezing the vertex is to be synonymous with making it static (never changing). If the user passes true for dynamic, that is like saying they will update the vertex buffer infrequently (aka GL_DYNAMIC_DRAW). If the user intends to update the vertex buffer every frame, then they should not call vertex_freeze just like in GMS (aka this indicates GL_STREAM_DRAW).

Optimization

This pull request wouldn't be complete if I didn't run some benchmarks. I clearly ran several games I have to check the performance, but the results I am going to share here are specifically from testing the GMS cubes demo.

Note: I used GPU-Z to monitor VRAM usage and I took the difference in VRAM occupation just before and after closing the game.
https://www.techpowerup.com/gpuz/

Some consistent issues that exist on master too that I want to note that this pr will not be fixing:

  • OpenGL1 transforms are screwed up
  • In all systems the lighting has an inconsistency with GMS (the projection functions should not update the light positions, they should just be updated in the draw call if dirty)
  • display_reset is broke in all of the working systems

Cubes Demo in ENIGMA DX9

Current Master

Branch Model create time (us) FPS RAM (MB) VRAM (MB)
GMS v1.4 520224 1430 175.0 53
ENIGMA GL1 062532 1533 076.4 55
ENIGMA GL3 078157 1557 087.6 56
ENIGMA DX9 078112 0658 093.7 38

This pull request unoptimized

Branch Model create time (us) FPS RAM (MB)
ENIGMA GL1 109387 18 093.9
ENIGMA GL3 109410 05 107.4+ (leaking)
ENIGMA DX9 109416 NA 090.8

Adjacent primitive combining

This optimization is fairly obvious. If you have two primitives of the same type that have the same vertex format adjacent in the model, you can simply treat them as a single primitive and thus make a single draw call. It was during this step that I had to provide a mechanism to hash the vertex formats for quick comparison.

This optimization is one of the reasons why a function like d3d_model_part_draw doesn't make sense, because it gets in the way of optimizing the model. Such functionality can be brought back of course by maintaining a mapping between the user specified ranges and what actually ends up in the model.

I saw some significant speedup in OpenGL1 from this optimization, but less so in Direct3D9 and OpenGL3 due to anomalies in their primitive batching (the fonts etc require a different kind of optimization).

Branch Model create time (us) FPS RAM (MB)
ENIGMA GL1 124989 1404 091.3
ENIGMA GL3 109381 970 301.3+ (leaking)
ENIGMA DX9 125010 15 099.6+ (leaking)

Vertex format caching

This optimization is also fairly obvious, but I actually didn't think it would bring the performance that it did. Originally I was trying to add vertex_format_delete as a function but gave up because my logic was too tricky. I decided to implement a cache for all of the user vertex formats using an std::unordered_map and the hashing value I already added.

As Rusky, Josh, and I discussed privately, this is better than expecting the user to have to worry about deleting and cleaning up vertex formats, which GMS doesn't. So you can freely call vertex_format_end as many times as you like, and you may just end up being given an already created vertex format.

Doing it in the general source is best because it doesn't require that I also add it to each system. Adding it for each peer vertex format would just be redundant, the D3D/OGL backends use the same ids.

Branch Model create time (us) FPS RAM (MB)
ENIGMA GL1 109385 1405 75.1
ENIGMA GL3 124985 0979 85.8
ENIGMA DX9 109373 1011 72.6

Vertex format peer caching

Caching the native peer (in this case the actual vertex declaration) gave another huge performance boost to D3D9. I didn't want to mess up D3D11 so I did not touch it. I will come back to D3D11 later on when I have projection functions working and benchmark how to optimize it. OpenGL1 uses fixed function pipeline and client state so there's no real way to cache the vertex format at all. OpenGL3 is still using a single default VAO and sort of doing what GL1 does, so I didn't want to mess with it yet, but I did get a nice boost by using the cached lookup function for an attribute location.

Branch Model create time (us) FPS RAM (MB) VRAM (MB)
ENIGMA GL1 093785 1406 75.1 50
ENIGMA GL3 109386 1220 86.1 72
ENIGMA DX9 109397 1483 69.4 38

Adjacent triangle strip combining using degenerates and buffer orphaning

I had reservations about doing this one, even though we did it before as well. Combining all primitive types together using an index buffer, which would also make fans work on mobile, is still an option. But in reality, there is nothing wrong in 2018 with using degenerate triangles to combine strips, even old graphics cards handle this really well.

During this phase, I discovered proper buffer orphaning and realized why TheExDeus had GL_STREAM_DRAW instead of GL_DYNAMIC_DRAW. Originally I did not properly understand the impact on performance this has when combined with buffer orphaning. By doing this first, I got a 50 fps boost in GL3, but then I realized instead of buffer orphaning we can just always call glBufferData because we never care about the previous contents of our buffer. By never calling glBufferSubData at all now, we avoid all synchronization overhead costs and I got an additional 50 fps boost in GL3 making the whole thing peak at 1607 max FPS. This is totally fine considering we don't have any intentions to support reading vertex buffers or updating subregions of them for now.

Branch Model create time (us) FPS RAM (MB) VRAM (MB)
ENIGMA GL1 🔻 093785 🔻 1403 ✔️ 75.1 ✔️ 42
ENIGMA GL3 🔻 109386 ✔️ 1607 🔻 88.5 ✔️ 40
ENIGMA DX9 🔻 109366 ✔️ 1711 ✔️ 69.6 38

RobertBColton added some commits Jul 29, 2018

@codecov

This comment has been minimized.

codecov bot commented Aug 8, 2018

Codecov Report

Merging #1289 into master will increase coverage by 0.22%.
The diff coverage is 10.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage    15.2%   15.43%   +0.22%     
==========================================
  Files         168      167       -1     
  Lines       17434    17380      -54     
==========================================
+ Hits         2651     2682      +31     
+ Misses      14783    14698      -85
Impacted Files Coverage Δ
...stem/SHELL/Graphics_Systems/General/GSmodel_impl.h 0% <0%> (ø)
...Asystem/SHELL/Graphics_Systems/General/GSmodel.cpp 0.16% <0.23%> (+0.16%) ⬆️
...tem/SHELL/Graphics_Systems/General/GSvertex_impl.h 72% <100%> (+13.17%) ⬆️
...system/SHELL/Graphics_Systems/OpenGL1/GLvertex.cpp 60% <39.13%> (-6.67%) ⬇️
...system/SHELL/Graphics_Systems/General/GSvertex.cpp 43.55% <55.35%> (+0.77%) ⬆️

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 e1e3e0a...a4d9b42. Read the comment docs.

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 11, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 11, 2018

@RobertBColton RobertBColton reopened this Aug 11, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 11, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 13, 2018

@RobertBColton RobertBColton force-pushed the generic-3d-models branch from 15737f4 to f736a85 Aug 14, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@JoshDreamland

This is looking to be a marvelous change. I need to review the principle file (GSmodel.cpp) a little more closely tomorrow. I'm too tired for it right now. I think that's where I'll find any problems with behavior (I'm skeptical of the way you're calling four other functions in each vertex function, and each of those is making the same if-then check and doing something expensive without falsifying the original conditional).

Anyway, I've left some other comments, mostly on readability, inline. I don't see any glaring problems with what you're doing, and the numbers don't lie, either.

@@ -113,15 +111,11 @@ int GetShapesModel() {
void BeginShapesBatching(int texId) {

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

This code's identical between the three systems; worth extracting it to Graphics_Systems/General?

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

It's absolutely worth extracting, and I already intended to do so. These 3 files here need deleted, and I intend to delete them all. But no, I will not do it in this pr because I do not want to conflate issues.

size_t spaces = 0;
bool trimmed = false;
bool checknormal = false;
for (unsigned int i = 0; i < s->size() ; i++)

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

Nit: This block is randomly formatted with opening braces on their own lines, and mismatched spacing around operators.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

I can fix it, but this was not my code, it was originally written by Tizzio on the forums. Obviously it has never been touched but I believe it has remained working because various people have claimed to load obj models with ENIGMA over the years. I did not really want to touch it and probably won't even if you beg me. Well, I had to touch the code at least a little to make it call d3d_model_vertex instead of the deleted AddVertex on Model struct.

std::vector<std::string> dat;
enigma::split(str, dat, ' ');
switch (atoi(dat[0].c_str())) {
case 0:

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

Readability: A static enum would make this switch statement infinitely more readable and cost nothing.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

Again, I don't know if you really want me to touch this when I didn't write it, out of fear of breaking it. It's just copied over from the old models class... verbatim.

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

Let's not do it now, at very least.

}
//obj model parsing functions
void string_parse(string *s) {

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

Git nit: If you extract the model save/load code to its own file, git may detect that this is a move operation (it may show the original file in the history).

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

It's not going to work now I don't think. I should have started by moving one of the original model sources first and then did the changes. It's not really worth it because this is otherwise an entirely new file. Correct me if I'm wrong.

model_static = 0, // GL_STATIC_DRAW D3DUSAGE_WRITEONLY
model_dynamic = 1, // GL_DYNAMIC_DRAW D3DUSAGE_DYNAMIC
model_stream = 2 // GL_STREAM_DRAW
model_static = 0, // write to buffer peer once (e.g, single time in the create event)

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

I think those constants are worth noting in the comment.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

Fair enough: 6970397

I mean, that is why I added them originally in the first place way back when.

}
}
GLvoid* graphics_prepare_buffer_array(const int buffer, const bool isIndex) {

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

This prototype is absolutely wonky. Please make this static (or put it in an anonymous namespace). Then it can have a shorter name. That bool isIndex thing just doesn't belong in an API.

This comment has been minimized.

@RobertBColton
// this is an "emulation" of vertex format declarations for the OpenGL fixed-function pipeline
switch (flag.second) {
case vertex_usage_position:
if (state.useVertices) break;
state.useVertices = true;
if (useVertices) break;

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

Why are you favoring your own state keeping instead of just asking GL for its actual state?

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

I didn't think calling glGet each iteration of the loop would be efficient. Also, it's not a matter of me wanting to do this, it's a requirement. I tested what GMS does, using its default shader, when you specify two colors in a vertex format. It only uses the first color that appears for drawing. So if I remove the bool check in my code, ENIGMA would use the last color, and we don't want it doing that. Because GL1 has no equivalent to IDirect3D9VertexDeclaration or a VAO, I could maybe have it cache a "flattened" copy of the vertex format that would be quicker to apply. That's unlikely to change our numbers on the cubes demo however.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

This sucks a bunch of ass: 740a263

Allow me to explain. glPushClientAttrib and glPopClientAttrib are really slow (totally killed the framerate), just like I suggested glGet would be, they involve a round trip to the GPU. Hence, you have to cache it yourself. I had to revert back to my even more verbose manual client state caching to speed back up my tiles benchmark that I didn't check until just now.

I looked high and low for an alternative here, and there's just no way we can avoid the Enable/Disable with our own caching, this is just how people do it. It allows other render functions (e.g, specifically d3d_draw_floor in Wild Racing GMK) to safely draw with vertex arrays which are faster than and just as expressive as immediate mode. It's just not good to leave certain client state enabled if we also want to allow user extensions to submit OpenGL calls to our context.

Now, I may have a way to avoid all of this in a future pr but it requires a lot of discussing. I am talking about moving GLprimitives.cpp with draw_primitive_begin and such over to General/GSprimitives.cpp for all of the systems. The functions would use the model class to define primitives and the model would be actually drawn and cleared (aka "flushed") whenever a render state change occurs (we already do this in GL3/DX9). Then we could totally cache the enabled client states since we won't have to worry about any other part of the system using vertex arrays. However, that would again be iffy if we want to allow extensions to submit GL calls to our context, but they could totally just "ask" us what the current vertex format is through API.

const GLvoid *data = isIndex ? (const GLvoid *)&indexBuffers[buffer]->indices[0] : (const GLvoid *)&vertexBuffers[buffer]->vertices[0];
if (size > (size_t)pSize || frozen) {

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

I assume the onus is now on the caller to only invoke graphics_prepare_buffer when something has changed? Might be worth noting that assumption, here.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

Absolutely not, graphics_prepare_buffer can safely be called every frame, and that's why it still does. It puts the GLuint etc into a map and when it isn't dirty it just binds the buffer peer to the buffer target. graphics_prepare_buffer is actually more like graphics_bind_buffer_and_prepare_if_dirty but that's a really long ass name for it 😛

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

Very nice, then.

@@ -34,6 +34,7 @@ D3DDECLUSAGE usage_types[] = { D3DDECLUSAGE_POSITION, D3DDECLUSAGE_COLOR, D3DDEC
map<int, LPDIRECT3DVERTEXBUFFER9> vertexBufferPeers;
map<int, LPDIRECT3DINDEXBUFFER9> indexBufferPeers;
map<int, std::pair<LPDIRECT3DVERTEXDECLARATION9, size_t> > vertexFormatPeers;
}

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

Mega nit: Going forward, try to comment these with } // namespace so it's clear it's not the end of an initializer list or function body.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

Good idea: 63759f8

I actually like these comments because it's hard for anybody really to tell where one namespace begins and another ends. I was actually telling fundies to do this too, so just, in general we should keep doing this.

std::hash<T> hasher;
seed ^= hasher(v) + 0x9e3779b9 + (seed<<6) + (seed>>2);
}
struct VertexFormat {
vector<pair<int,int> > flags;

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 14, 2018

Member

This is confusing and I never found docs for it (the only reason I know what this is is because I see you using them to switch client capabilities on and off in GL). The key is clearly in the realm of "enum material," especially since this header is in General where the type won't be specific to one system.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 14, 2018

Member

I'm sorry, this is straight out of GMS which doesn't document it well.

There is an enum for the types and usages in GSvertex.h and I have documented them already on ENIGMA's wiki (though some of my newer vertex functions need updated).
https://enigma-dev.org/docs/Wiki/Vertex_Functions

I just did that documentation recently too, so it's very very thorough. You can find out what the constants are by looking at this doc:
https://enigma-dev.org/docs/Wiki/Vertex_format_add_custom
https://enigma-dev.org/docs/Wiki/Vertex_constants

We deal with the usage semantics in GL by enforcing that the user use specific names for their shader variables (e.g, "in_Position0-9"). I had a whole conversation on Discord with @rpjohnst about this. And @TheExDeus already had the GL3 shader code laid out like that.

Are you wanting me to make the enum into a class enum so I can refer to it by name instead of int? If we're going to do that, why not do it for the rest of ENIGMA? (I'm cool witdat)

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 14, 2018

@JoshDreamland

Fantastic job on this, and especially good work responding to those comments. I am happy for this to submit in its current state.

@@ -19,37 +19,66 @@
#define ENIGMA_GSMODEL_H
#include "Universal_System/scalar.h"
#include "Universal_System/dynamic_args.h" // for d3d_model_vertex_data

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

However you like it is fine.

@@ -39,16 +41,26 @@ vector<VertexFormat*> vertexFormats;
vector<VertexBuffer*> vertexBuffers;
vector<IndexBuffer*> indexBuffers;
std::unordered_map<size_t, int> vertexFormatCache;
VertexFormat* vertexFormat = 0;

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

I might move this to an anonymous namespace to keep it from polluting other files. Also, it's the current vertex format, right? I'd comment that, if not rename.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 15, 2018

Member

Done: f4ee307

Yeah, vertexFormat is the current vertex format the user is specifying between calls to vertex_format_begin and vertex_format_end. It is guaranteed not to exist outside of that scope.

struct VertexFormat {
vector<pair<int,int> > flags;
std::size_t stride;
std::size_t stride, hash;

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

The issue is that you're initializing it to 0 and then trying to remember to update it every time someone mutates this. You're likely to fail that way, so if it's not saving us any compute, I'd ditch it. But from the sounds of things, it may be.

Unrelated thing to test: how many unique hashes are there vs how many unique formats, after your cubes demo has been running for a few frames?

int format;
std::size_t number;
VertexBuffer(): frozen(false), dirty(false), format(-1), number(0) {}
int getNumber() const {
return dirty ? vertices.size() : number;

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

I see. Good to know.

const GLvoid *data = isIndex ? (const GLvoid *)&indexBuffers[buffer]->indices[0] : (const GLvoid *)&vertexBuffers[buffer]->vertices[0];
if (size > (size_t)pSize || frozen) {

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

Very nice, then.

unsigned vertex_get_size(int buffer);
unsigned vertex_get_number(int buffer);
void vertex_freeze(int buffer);
void vertex_freeze(int buffer, bool dynamic = false);

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

I'm not clear on why this takes dynamic as a boolean. Also, is it worth exposing the actual background types (static, dynamic, stream) as constants, rather than just having them specify bool dynamic? Is it stream/dynamic by default, and then set to static/dynamic when frozen?

This comment has been minimized.

@RobertBColton

RobertBColton Aug 15, 2018

Member

I can't really say if it's worth abstracting those types. You see, vertex_freeze is only here for compatibility with GMS. That allegedly marks it as readonly, and I assume that means they make it a static buffer.
https://docs.yoyogames.com/source/dadiospice/002_reference/shaders/primitive%20building/vertex_freeze.html

Here's the meaning:

  • GL_STATIC_DRAW: call vertex_freeze with dynamic false
  • GL_DYNAMIC_DRAW: call vertex_freeze with dynamic true
  • GL_STREAM_DRAW: don't ever call vertex_freeze

So that makes it compatible with GMS and has a nice parallel with how their API does it. That lets us also provide our users the ability to hint that although they are going to freeze the vertex buffer, they want to be able to update it infrequently.

std::vector<std::string> dat;
enigma::split(str, dat, ' ');
switch (atoi(dat[0].c_str())) {
case 0:

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

Let's not do it now, at very least.

}
void d3d_model_draw(int id, gs_scalar x, gs_scalar y, gs_scalar z) {
d3d_transform_add_translation(x, y, z);

This comment has been minimized.

@JoshDreamland

JoshDreamland Aug 15, 2018

Member

GL1 may perform better if you use d3d_transform_push_translation(x, y, z) / d3d_transform_pop(). Don't feel obligated to try this before submitting this change.

This comment has been minimized.

@RobertBColton

RobertBColton Aug 15, 2018

Member

That will break Direct3D9 where I haven't written those transform stack functions yet, we can look at doing that later and benchmark it. 👍

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 15, 2018

@enigma-dev enigma-dev deleted a comment from EnigmaBot Aug 15, 2018

@RobertBColton RobertBColton merged commit 16807a7 into master Aug 15, 2018

3 of 4 checks passed

codecov/patch 10.88% of diff hit (target 15.2%)
Details
codecov/project 15.43% (+0.22%) compared to e1e3e0a
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-3d-models branch Aug 15, 2018

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

Generic 3D Models (#1289)
* Every old model class removed and deleted from git
* A new generic model class was created to replace the deleted ones
* Changed `d3d_model_create` to return an int like other resource functions for consistency
* Removed `d3d_model_part_draw` functions because their name is inconsistent (e.g, should be `draw_part` like `draw_background_part`) and because they are not clear enough for a model class which optimizes and combines primitives together, `d3d_model_draw_primitive` with an index to the primitive would be a better function. We also no longer use these functions anywhere, tiles use the vertex and index buffer abstractions directly now which offer range arguments in their submit functions.
* Added a new function `d3d_model_vertex_data` that allows one to specify the model's vertex data in a single function call, but requires a vertex format be provided.
* Updated `GSmodel.h` with comments specifying the preconditions for each group of model specification functions
* Renamed `d3d_model_add_*` functions to remove the `_add_` part of their name to match the original `d3d_model_vertex` function from GM for consistency.
* Added the following new vertex functions:
    * `vertex_format_exists()`: This non-id version of the function will tell us if we are between `vertex_format_begin` and `vertex_format_end` calls. In other words, it tells us if we are still defining a new vertex format.
    * `vertex_format_get_stride()`: Non-id version returns the stride of the vertex format currently being specified.
    * `vertex_format_get_hash()`: Non-id version returns the hash of the vertex format currently being specified.
    * `vertex_format_get_stride(id)`: Returns the stride of the vertex format with the given id. This is basically the number of vertex attributes, it is not the size of a vertex in bytes.
    * `vertex_format_get_hash(id)`: Returns a hash of the vertex format and its type/usage pairs.
    * `vertex_set_format(buffer,format)`: Allows one to change the vertex format of a vertex buffer, which essentially allows the vertex buffer to hold the same data for differently formatted primitives.
* Made `vertex_format_end` set the `enigma::vertexFormat` pointer to 0 so there can be no further accidental mutation of it once it is moved to the vector and assigned a real id for the user.
* Made `vertex_get_number`, `vertex_get_size`, and their index buffer equivalents return the correct value when in the middle of `vertex_begin` and `vertex_end` which previously they would not. This required marking the vertex and index buffers dirty in `vertex_begin` instead of `vertex_end` so that the dirty flag could be used to determine where to grab the number of vertices/indices from.
* Added a non-fatal error message for `vertex_data` that shows in debug mode if the user calls the function but the vertex buffer does not have a valid vertex format, since one is required for the function to correctly decompose the arguments.
* Removed all `d3d_model_has_*` functions because we aren't using them anywhere and we don't need them.
* Changed `graphics_reset_client_state` to unbind the vertex and index buffers (bind the target to 0) in OpenGL1 because it will screw up primitive rendering that uses `glVertexPointer` with a client-side array. This caused issues in Wild Racing GMK because `d3d_draw_floor` was not being drawn and I could not figure out why. 
* Removed `d3d_model_get_stride` and `d3d_model_format` as well as calls to them in the context managers in Bridges because the function no longer makes sense. The model class in this pr has a format for each primitive, hence the entire model doesn't have a consistent stride or format.
* Removed `d3d_model_index` because the new model class doesn't support indexing yet.
* Added caching of both vertex formats and vertex format peers. The peers are non-trivial to construct and we also don't want the user being wasteful with the former either. Caching both independently gave separate performance boosts, so both are necessary. Because of this change, `vertex_format_end` may now return the same id multiple times and never actually return a unique id if the hashes of the vertex format specified are always logically equivalent to another format.
* Fixed a typo in `d3d_model_set_rotation_x` for D3D9, it should have had `gs_scalar` not `double` like its signature in the general header.
* Added a `dynamic` flag to `vertex_freeze` so the user can indicate such usage. By default I set the parameter to false, because freezing the vertex is to be synonymous with making it static (never changing). If the user passes true for dynamic, that is like saying they will update the vertex buffer infrequently (aka GL_DYNAMIC_DRAW). If the user intends to update the vertex buffer every frame, then they should not call `vertex_freeze` just like in GMS (aka this indicates GL_STREAM_DRAW).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment