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

Queued dxDrawPrimitive and dxDrawMaterialPrimitive #339

Merged
13 commits merged into from Aug 30, 2018

Conversation

@forkerer
Copy link
Collaborator

@forkerer forkerer commented Aug 24, 2018

It's pretty much this pull request, but with proper queueing, state saving, and ability to be called from anywhere, not only onClientRender event.
Also there's added postGUI parameter:

bool dxDrawPrimitive( string primitiveType, [ postGUI = true ] table vertex1, table vertex2, table vertex3, ... )

bool dxDrawMaterialPrimitive( string primitiveType, material, [ postGUI = true ] table vertex1, table vertex2, table vertex3, ... )

@CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented Aug 24, 2018

HE STOLE MY PULL REQUEST!

@forkerer
Copy link
Collaborator Author

@forkerer forkerer commented Aug 24, 2018

yes

@4O4
Copy link
Contributor

@4O4 4O4 commented Aug 24, 2018

While I really appreciate the work you did, I have to say the way you did it feels really wrong. This should be kept in scope of one pull request - you could ask @CrosRoad95 for write permissions to his fork or send him a patch to apply manually. But this is the tiny issue here. The bigger one is the legal one - code might be open source but there's a thing called copyright. You two should have a short conversation and come to some agreement :)

@forkerer
Copy link
Collaborator Author

@forkerer forkerer commented Aug 25, 2018

Ye, i completely agree that it isn't the best way of doing it. My motivation for that was that i was telling CrosRoad exactly what's wrong with his dxDrawPrimitive since he created the pull request, as it is just flawed. I wont mind if this request is closed and not accepted as long as the original one is modified to work properly. It's not an attack on him, i just want the primitive drawing to be added, and this is the easiest way for me to show how it could be done.

@CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented Aug 25, 2018

i can close my pr and you can marge this one.
also i noticed that your function don't have any limit of vertices, i tested and limit 4096 vertices should be ok

@CrosRoad95
Copy link
Contributor

@CrosRoad95 CrosRoad95 commented Aug 25, 2018

i have one question: should be option to rotate and scale primitives or not? just like dxDrawText support this

@forkerer
Copy link
Collaborator Author

@forkerer forkerer commented Aug 25, 2018

Would be pretty complicated to add right now i think, it's a good idea tho. Additional rotate/scale center parameter sounds nice.

Kamil Marciniak added 3 commits Aug 25, 2018
- dxDrawPrimitive doesn't require you to draw at least 2 points now
- Batcher classes did draw only a single point at all times anyways tho, fixed that
- Cleaned up CPrimitiveBatcher.cpp
- Replaced NULLs with nullptrs
Noticed that with last commit it would be possible to try draw 0 points without error
@qaisjp
Copy link
Contributor

@qaisjp qaisjp commented Aug 26, 2018

#301 has more gifs and so is better /close (thanks for your hard work @CrosRoad95)

@ghost ghost changed the title Queued dxDrawPrimitive Queued dxDrawPrimitive and dxDrawMaterialPrimitive Aug 26, 2018
Copy link

@ghost ghost left a comment

The methods are too long in your PR, you can very easily shorten them. If your methods are shorter, your code will be readable and thus more easier to maintain.

.gitignore Outdated
@@ -416,3 +416,7 @@ utils/DXFiles/
!*.dll

This comment has been minimized.

@ghost

ghost Aug 26, 2018

.gitignore is not needed, you can delete it.

// Make projection 3D friendly, so shaders can alter the z coord for fancy effects
float fFarPlane = 10000;
float fNearPlane = 100;
float Q = fFarPlane / (fFarPlane - fNearPlane);

This comment has been minimized.

@ghost

ghost Aug 26, 2018

what is Q? Try to use pronounceable names for variables, because it will be really hard to understand the purpose later.


for (int i = 0; i < m_primitiveList.size(); i++)
{
sDrawQueuePrimitiveMaterial primitive = m_primitiveList[i];

This comment has been minimized.

@ghost

ghost Aug 26, 2018

why not use (auto& primitive : m_primitiveList)?

m_pDevice->SetRenderState(D3DRS_ALPHATESTENABLE, TRUE);
m_pDevice->SetRenderState(D3DRS_ALPHAREF, 0x01);
m_pDevice->SetRenderState(D3DRS_ALPHAFUNC, D3DCMP_GREATEREQUAL);
m_pDevice->SetRenderState(D3DRS_LIGHTING, FALSE);

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Create a new function void CPrimitiveMaterialBatcher::SetRenderStates (), call m_pDevice->SetRenderState within that function.

m_pDevice->SetTextureStageState(0, D3DTSS_ALPHAARG1, D3DTA_TEXTURE);
m_pDevice->SetTextureStageState(0, D3DTSS_ALPHAARG2, D3DTA_DIFFUSE);
m_pDevice->SetTextureStageState(1, D3DTSS_COLOROP, D3DTOP_DISABLE);
m_pDevice->SetTextureStageState(1, D3DTSS_ALPHAOP, D3DTOP_DISABLE);

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Create a new function void CPrimitiveMaterialBatcher::SetTextureStageStates (), call m_pDevice->SetTextureStageState within that function.

return 1;
}

switch (primitiveType)

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Use this code here:

if (IsPrimitiveSizeIncorrect (primitiveType, vecVertices))
{
    lua_pushboolean(luaVM, false);
    return 1;
}

// Draw
m_pDevice->SetTexture(0, nullptr);
for (int i = 0; i < m_primitiveList.size(); i++)

This comment has been minimized.

@ghost

ghost Aug 26, 2018

why not use (auto& primitive : m_primitiveList)?

case D3DPT_LINESTRIP:
iSize = iCollectionSize - 1;
break;
case D3DPT_TRIANGLEFAN:

This comment has been minimized.

@ghost

ghost Aug 26, 2018

case D3DPT_TRIANGLEFAN:
    break;

This comment has been minimized.

@forkerer

forkerer Aug 26, 2018
Author Collaborator

D3DPT_TRIANGLEFANsize needs to be same as D3DPT_TRIANGLESTRIP, so break is unnecessary, it uses shared case

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Oh, that makes sense.

m_pDevice->SetRenderState(D3DRS_ALPHATESTENABLE, TRUE);
m_pDevice->SetRenderState(D3DRS_ALPHAREF, 0x01);
m_pDevice->SetRenderState(D3DRS_ALPHAFUNC, D3DCMP_GREATEREQUAL);
m_pDevice->SetRenderState(D3DRS_LIGHTING, FALSE);

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Create a new function void CPrimitiveMaterialBatcher::SetRenderStates (), call m_pDevice->SetRenderState within that function.

m_pDevice->SetTextureStageState(0, D3DTSS_ALPHAOP, D3DTOP_SELECTARG2);
m_pDevice->SetTextureStageState(0, D3DTSS_ALPHAARG2, D3DTA_DIFFUSE);
m_pDevice->SetTextureStageState(1, D3DTSS_COLOROP, D3DTOP_DISABLE);
m_pDevice->SetTextureStageState(1, D3DTSS_ALPHAOP, D3DTOP_DISABLE);

This comment has been minimized.

@ghost

ghost Aug 26, 2018

Create a new function void CPrimitiveMaterialBatcher::SetTextureStageStates (), call m_pDevice->SetTextureStageState within that function.

@ccw808
Copy link
Member

@ccw808 ccw808 commented Aug 26, 2018

sam1ler, the code you commented on has been copied from CTileBatcher.cpp, so I don't think your suggestions should be done in isolation.

@ghost
Copy link

@ghost ghost commented Aug 26, 2018

@ccw808 I see, I didn't know that. This is a lot of code. We have a problem of redundancy here. How about sharing the code between CTileBatcher.cpp, CPrimitiveMaterialBatcher.cpp, and CPrimitiveBatcher.cpp? I can refactor the code in CTileBatcher.cpp, then the author of PR can make appropriate changes to the PR.

@ccw808
Copy link
Member

@ccw808 ccw808 commented Aug 26, 2018

@sam1ler it's up to you. (I would put refactor on the TODO list and leave it for now)

@qaisjp
Copy link
Contributor

@qaisjp qaisjp commented Aug 29, 2018

When merging (after 1.5.6), please squash and add the following line to the end of the commit message:

Co-authored-by: CrosRoad95 <sebajura1234@gmail.com>
@forkerer forkerer force-pushed the forkerer:dxDrawPrimitive branch from e1576a3 to e54d345 Aug 30, 2018
@ghost ghost merged commit 893c134 into multitheftauto:master Aug 30, 2018
3 checks passed
3 checks passed
@wip
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@patrikjuvonen patrikjuvonen assigned ghost Sep 4, 2018
@patrikjuvonen patrikjuvonen modified the milestones: 1.5.6, 1.5.7 Sep 4, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants