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

Indexed rendering? #254

Closed
ocornut opened this issue Jun 30, 2015 · 21 comments
Closed

Indexed rendering? #254

ocornut opened this issue Jun 30, 2015 · 21 comments
Labels

Comments

@ocornut
Copy link
Owner

ocornut commented Jun 30, 2015

Asking for feedback.

  • I would like to switch to indexed rendering. There's rather good benefits in term of CPU/GPU/memory cost and it merged within the anti-aliased branch (Anti-aliased rendering #133) it makes the cost of the anti-aliasing really cheaper.

There's up to 4 render code-paths I could potentially maintain:

  • non anti-aliased non indexed
  • non anti-aliased indexed
  • anti-aliased non indexed
  • anti-aliased indexed
    (They are relatively small functions)

But if I could drop non-indexed it'll be just simpler (if I drop non-indexed it's also more doable to drop non-anti aliased).

  • That'll effectively break everybody's renderer but the fix (to use indexed rendering) should be relatively trivial.
    Can you think of a case where indexed rendering would be a hassle or undoable for anyone? with certain API, under certain hardware?
@extrawurst
Copy link
Contributor

If you ask me: just do it - the concept of indexed rendering is older than the most of us and I personally cannot think of anything that could break. then again i am not abusing the possibility to intersect the rendering at all, I am just rendering what imgui gives me straight away - but IMO thats what most of us do...

@hypernewbie
Copy link

+1 do it

@MrSapps
Copy link

MrSapps commented Jun 30, 2015

Unless someone has some weird hand made software engine can't think of any reason why it wouldn't be supported.

@phicore
Copy link

phicore commented Jun 30, 2015

+1 also

@emoon
Copy link
Contributor

emoon commented Jun 30, 2015

Fine with me also.

@ocornut
Copy link
Owner Author

ocornut commented Jun 30, 2015

Alright, this is making my life simpler. I realise everybody here is super biased because you are the most likely people to be up to date, so I am still cautious but will proceed.

The anti-aliasing branch now has indexed rendering merged in
#133

So at least when I announce "you have to fix your render functions" it'll come with a "but you get anti-aliasing".

@emoon
Copy link
Contributor

emoon commented Jun 30, 2015

If you really wanted to you could have a compatibility mode that just does a "de-indexing" pass before calling the supplied backend call for the rendering. That would allow you to keep all code indexed based except that bit.

@ocornut
Copy link
Owner Author

ocornut commented Jun 30, 2015

That's true. While I am breaking the render function I will take the occasion to change the parameter from (ImDrawList** const draw_lists, int count) to a structure that can contains more data. Then the structure can totally hold a helper to do the dei-ndexing.

@emoon
Copy link
Contributor

emoon commented Jul 1, 2015

Yeah that is what I was thinking. So perhaps you can have a define (or a setting) that can enable compat mode for those who needs it while it's being phased out after x releases.

@ebriney
Copy link

ebriney commented Jul 1, 2015

Indexed is better for GPU and CPU, as soon as your primitives are quads or real 3d models of course !!!
And i don't know any graphic card that doesn't support the 16 bits indexed triangles...
My advice, change the command interface so it generates an error at compile time and the fix is really trivial -> bind an indexed buffer and change the draw call to indexed version

@emoon
Copy link
Contributor

emoon commented Jul 1, 2015

This (imo) isn't about whatever GPU or CPU path supports indexing. It's about a API change that will break for everyone using the API so I rather go with a more pragmatic approach of phasing something out instead of giving people compile errors.

@extrawurst
Copy link
Contributor

api changes happened in a lot of the recent versions. an imgui user is
kinda used to this, so i think we should not start to overcomplicate things
now with this change where support old and new api is especially more work.

On Wed, Jul 1, 2015 at 1:11 PM, Daniel Collin [email protected]
wrote:

This (imo) isn't about whatever GPU or CPU path supports indexing. It's
about it's an API change that will break for everyone using the API so I
rather go with a more pragmatic approach of phasing something out instead
of giving people compile errors.


Reply to this email directly or view it on GitHub
#254 (comment).

@emoon
Copy link
Contributor

emoon commented Jul 1, 2015

Like I wrote above. All that is needed is a de-indexing pass just before call to user supplied renderFunc which is not that many lines. The rest of the code can just stick with building everything the indexed way.

@ocornut
Copy link
Owner Author

ocornut commented Jul 1, 2015

extrawurst

api changes happened in a lot of the recent versions. an imgui user is kinda used to this, so i think >we should not start to overcomplicate things now with this change where support old and new api is >especially more work.

Yes but the majority of api changes involved recently introduced functions so you would have to be recently up to date AND using those features to be affected. The problem here is that this change will affect 100% of the users.

Regardless I'll do what Daniel say and just add a helper to "unindex" the vertices at a later stage. I will be slower obviously but at least won't require any maintenance in the main code. Also it won't be by default ! By default I'll make sure that old code wiill break and let the user choose between implementing indexed rendering or calling that function.

@extrawurst
Copy link
Contributor

ok sounds good to me

@emoon
Copy link
Contributor

emoon commented Jul 1, 2015

+1

@paultech
Copy link

paultech commented Jul 1, 2015

+1 for default index rendering with "unindex" helper. I'm personally fine with even large API changes so long as they are documented that I don't have to chase down issues from a unnoticed change.

@ocornut
Copy link
Owner Author

ocornut commented Jul 2, 2015

Here's the function

// For backward compatibility: convert all buffers from indexed to de-indexed, in case you cannot render indexed. Note: this is slow and most likely a waste of resources. Always prefer indexed rendering!
void ImDrawList::DeIndexBuffers()
{
    if (idx_buffer.empty())
        return;
    ImVector<ImDrawVert> new_vtx_buffer;
    new_vtx_buffer.resize(idx_buffer.size());
    for (size_t i = 0; i < idx_buffer.size(); i++)
        new_vtx_buffer[i] = vtx_buffer[idx_buffer[i]];
    vtx_buffer.swap(new_vtx_buffer);
    idx_buffer.resize(0);
}

It's not added yet (but I tested it) because it'll be added not in ImDrawList but in the new structure that'll get passed to rendering. Closing this but I have the reference code.

@ocornut ocornut closed this as completed Jul 2, 2015
ocornut added a commit that referenced this issue Jul 6, 2015
… neater and future proof breaking of the render API (#133 #254)
ocornut added a commit that referenced this issue Jul 6, 2015
@ocornut
Copy link
Owner Author

ocornut commented Jul 6, 2015

New API has been added to the AA-branch, see #133

@MrSapps
Copy link

MrSapps commented Dec 28, 2016

Is there still an option/way to use this DeIndexBuffers? I couldn't see DeIndexBuffers in latest code. I'm adding ImGui via hooking to a super old engine that doesn't use indexed rendering so I want the old school ways ;). If its now 100% gone then I can just de-index myself.

@ocornut
Copy link
Owner Author

ocornut commented Dec 28, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants