xrRender: static geometry buffers code unification (#489) #490

vTurbine · 2019-10-11T22:35:35Z

Added staging buffer wrapper for static geometry;
Common buffer interface for DX9/DX10/DX11/VK
Backend top file simplified and made common

Please check if it works fine on Linux as well: I don't have OpenGL 4.x on my VM

Xottab-DUTY · 2019-10-12T03:18:26Z

AppVeyor failed to compile this PR =(

vTurbine · 2019-10-12T05:47:49Z

Right, sorry. BufferUtils.h was lost in pile of untracked files. Re-pushed with it

vTurbine · 2019-10-13T13:14:40Z

It turned out that after this modification we able to share some of render targets files(!) across R2/R3/R4/GL since it became fully portable. Also this can give a good boost for #492

src/Layers/xrRenderDX10/dx10BufferUtils.cpp

SkyLoaderr · 2019-10-14T16:40:24Z

Has anyone tested this PR on Linux?

SkyLoaderr · 2019-10-14T16:50:16Z

src/Layers/xrRenderPC_R2/r2_loader.cpp

- R_CHK(HW.pDevice->CreateIndexBuffer(iCount * 2, dwUsage, D3DFMT_INDEX16, D3DPOOL_MANAGED, &_IB[i], 0));
- HW.stats_manager.increment_stats_ib(_IB[i]);
- R_CHK(_IB[i]->Lock(0, 0, (void**)&pData, 0));
+ _IB[i].Create(iCount * 2);


Old code doesn't use D3DUSAGE_SOFTWAREPROCESSING for dwUsage in software mode, but _IB[i].Create has it.

This odd for level geometry loading only. I believe this typo came from past times and the flag should be set. See R1 for reference.
Any reasons to treat this particular geometry buffers in different manner? Am I missing smth?

This odd for level geometry loading only. I believe this typo came from past times and the flag should be set. See R1 for reference.
Any reasons to treat this particular geometry buffers in different manner? Am I missing smth?

@SkyLoaderr, so, do you have something to say about it?

I bet this is not a typo and was made intentional. But, however, R2 probably won't even launch without hardware features, so this should be safe.

src/Layers/xrRender/BufferUtils.h

src/Layers/xrRenderDX9/CommonTypes.h

src/Layers/xrRender/BufferUtils.h

src/Layers/xrRenderPC_R2/r2_rendertarget_accum_point_geom.cpp

src/Layers/xrRenderPC_R2/r2_rendertarget_accum_spot_geom.cpp

src/Layers/xrRender/BufferUtils.h

007exe · 2019-10-25T19:24:08Z

I checked on ArchLinux in conjunction with the GeForce 9600 GT on proprietary drivers, I did not notice any new bugs

* Dead code restored * `GetHostPointer` -> `Map` * Option to keep host buffer for further readings * `Destroy` is hidden to prevent ref counter miscalculations

vTurbine · 2019-10-26T19:05:43Z

Thank you for review. Let's decide now how to solve the issue with naming.
Two use cases are possible:

Lock(map?) host buffer to fill the buffer obtained with data and then upload it to GPU (unlock + flush)
Lock host buffer again to read data back and then unlock without flushing since no content changed

It will end up with this patterns:
1.

Create(size);
ptr = Lock();
< fill ptr >
Unlock();
Flush();

Create(size, true);
ptr = Lock();
< fill ptr >
Unlock();
Flush();
...
ptr = Lock(true); // for read
Unlock();

So,

should I add explicit Unlock after each buffer use?
isn't map/unmap names fit better for case 2?
Codacy reports wrongly scoped variables came from vanilla code. Do they need to be fixed too?

Xottab-DUTY · 2019-10-26T19:38:02Z

isn't map/unmap names fit better for case 2?

Since it doesn't actually Lock/Unlock buffers on DX10+/OpenGL and we want to deprecate DX9 (which is the only implementation which actually does Lock/Unlock) it is better to Map/Unmap, rather than Lock/Unlock.

should I add explicit Unlock after each buffer use?

Summing up the above and the fact that Unmap() does nothing on DX10+/OpenGL, we can just use Map(), Unmap() and FlushAndUnmap(), so no explicit Unlock() calls are required.

Eventually, it can look like that:
1.

Create(size);
ptr = Map();
< fill ptr >
FlushAndUnmap();

Create(size, true);
ptr = Map();
< fill ptr >
FlushAndUnmap();
...
ptr = Map(true); // for read
Unmap();

The main thing with my suggestion is that then Map, Unmap and FlushAndUnmap functions should be documented well.

Layers/xrRender/BufferUtils.h should contain behaviour description for all implementations (DX9/DX10+/OpenGL)

API specific implementations Layers/xrRender*/*BufferUtils.cpp should contain behaviour description specific to that API.

Codacy reports wrongly scoped variables came from vanilla code. Do they need to be fixed too?

Will fix that myself)

Xottab-DUTY · 2019-10-27T08:19:45Z

AppVeyor compilation failed

vTurbine · 2019-10-27T08:47:28Z

Summing up the above and the fact that Unmap() does nothing on DX10+/OpenGL, we can just use Map(), Unmap() and FlushAndUnmap(), so no explicit Unlock() calls are required.

Will be Unmap(doFlush=true) easier then?

Create(size);
ptr = Map();
< fill ptr >
Unmap(true); // flush

Create(size, true); // read capable
ptr = Map();
< fill ptr >
Unmap();
...
ptr = Map(true); // for read
< read ptr>
Unmap();
...
DiscardHostBuffer();

Xottab-DUTY · 2019-10-27T09:40:27Z

Will be Unmap(doFlush=true) easier then?

Yes, it will.

- `Flush` removed; - More verbosity in assertions; - Don't allow to map buffer for writes twice. Such pattern isn't used in the Engine and this will need buffer handle invalidation. - Some documentation added

vTurbine · 2019-10-31T14:25:59Z

Sorry for late update: my PC burnt out and it will take a couple of days to bring the next changes

Xottab-DUTY · 2019-10-31T15:09:21Z

Hope you will successfully recover your PC! 😃

src/Layers/xrRenderGL/glBufferUtils.cpp

Xottab-DUTY

@vTurbine, anything more to apply or we may merge?

vTurbine · 2019-11-04T08:00:08Z

It's fine, no more changes on this step.
BTW, I removed the code which restores pre-computed indices in QuadIB (see CBackend::RestoreQuadIBData()). Not sure if anyone uses it so I'm going to remove it completely right after this PR merging. Is it ok or I need to return this back?

Xottab-DUTY · 2019-11-04T09:54:36Z

It's better to just remove it right in this PR. Why removing in two steps? (partial code removing and then full removal in other PR)
We can and it's better to remove it right now.

Some history:
CBackend::RestoreQuadIBData() related to the old bug with rain particles corruption, many programmers tried to fix it in SoC and CS. (the bug appeared even before SoC was released)
It was finally fixed by Armen Abroyan in CoP.

vTurbine · 2019-11-04T11:24:41Z

Thank you for explanation.
Done. Will keep an eye on regression

Xottab-DUTY · 2019-11-04T11:30:45Z

It was finally fixed by Armen Abroyan in CoP.

The fix is simple. Here's my backport to Clear Sky: OpenXRay/xray-15@b8fd8b9

vTurbine added 8 commits October 11, 2019 22:16

xrRender: added index staging buffer abstraction

f47a8b2

xrRender: Backend: top file cleanup

2df13e1

xrRender: added vertex staging buffer abstraction

c9653e6

xrRender: Details: detail manager switched to staging buffers

64d9691

xrRender: vertex format helpers interface unified

3804d24

xrRender: Utils: geometry switched to staging buffers

6f436ae

xrRender: BufferUtils: staging buffer validation added

3057e28

xrRender: BufferUtils: staging buffer is reusable now

97b0b8e

Xottab-DUTY added Portability Renderer labels Oct 12, 2019

Xottab-DUTY requested a review from SkyLoaderr October 12, 2019 03:23

xrRender: renderers were switched to common BufferUtils.h

31056ec

vTurbine added 5 commits October 13, 2019 21:24

xrRender: BufferUtils: added ref counting into staging buffer

097463e

xrRenderR2: RT switched to staging buffers

df0d27a

xrRenderR2: loader switched to staging geometry buffers

3d38723

xrRenderR1: loader switched to staging geometry buffers

42d1d22

xrRenderR3: loader switched to staging geometry buffers

b1cacab

SkyLoaderr reviewed Oct 14, 2019

View reviewed changes

src/Layers/xrRenderDX10/dx10BufferUtils.cpp Outdated Show resolved Hide resolved

xrRenderR4: loader switched to staging buffers

a837eb6

SkyLoaderr reviewed Oct 14, 2019

View reviewed changes

src/Layers/xrRender/BufferUtils.h Outdated Show resolved Hide resolved

Xottab-DUTY reviewed Oct 14, 2019

View reviewed changes

src/Layers/xrRenderDX9/CommonTypes.h Outdated Show resolved Hide resolved

src/Layers/xrRender/BufferUtils.h Outdated Show resolved Hide resolved

Review iteration

861de26

Xottab-DUTY reviewed Oct 24, 2019

View reviewed changes

Review iteration #2

f84a3d0

* Dead code restored * `GetHostPointer` -> `Map` * Option to keep host buffer for further readings * `Destroy` is hidden to prevent ref counter miscalculations

Assert statement simplified

7c5b1c7

Merge branch 'xd_dev' into renderer/staging_buffers

3b3a6a3

Merge issue resolved

f17ab22

Review iteration #3

3732d5a

- `Flush` removed; - More verbosity in assertions; - Don't allow to map buffer for writes twice. Such pattern isn't used in the Engine and this will need buffer handle invalidation. - Some documentation added

Xottab-DUTY reviewed Nov 1, 2019

View reviewed changes

src/Layers/xrRenderGL/glBufferUtils.cpp Outdated Show resolved Hide resolved

Coding style fixed

1ebd608

Xottab-DUTY approved these changes Nov 3, 2019

View reviewed changes

r_restore_quad_ib_data removed

794696d

Xottab-DUTY merged commit 9086964 into OpenXRay:xd_dev Nov 4, 2019

vTurbine deleted the renderer/staging_buffers branch November 4, 2019 11:41

Xottab-DUTY added a commit that referenced this pull request Nov 7, 2019

xrRender: fixed broken device reset on DX9 after #490

3964a31

vTurbine mentioned this pull request Nov 8, 2019

Render: Broken index buffer after vid_restart #514

Closed

eagleivg pushed a commit to eagleivg/xray-16 that referenced this pull request Nov 11, 2019

xrRender: fixed broken device reset on DX9 after OpenXRay#490

b9e6661

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xrRender: static geometry buffers code unification (#489) #490

xrRender: static geometry buffers code unification (#489) #490

vTurbine commented Oct 11, 2019

Xottab-DUTY commented Oct 12, 2019

vTurbine commented Oct 12, 2019

vTurbine commented Oct 13, 2019 •

edited

Loading

SkyLoaderr commented Oct 14, 2019

SkyLoaderr Oct 14, 2019

vTurbine Oct 14, 2019

Xottab-DUTY Nov 1, 2019

Xottab-DUTY Nov 3, 2019 •

edited

Loading

007exe commented Oct 25, 2019

vTurbine commented Oct 26, 2019

Xottab-DUTY commented Oct 26, 2019 •

edited

Loading

Xottab-DUTY commented Oct 27, 2019

vTurbine commented Oct 27, 2019

Xottab-DUTY commented Oct 27, 2019

vTurbine commented Oct 31, 2019

Xottab-DUTY commented Oct 31, 2019

Xottab-DUTY left a comment

vTurbine commented Nov 4, 2019

Xottab-DUTY commented Nov 4, 2019

vTurbine commented Nov 4, 2019

Xottab-DUTY commented Nov 4, 2019

xrRender: static geometry buffers code unification (#489) #490

xrRender: static geometry buffers code unification (#489) #490

Conversation

vTurbine commented Oct 11, 2019

Xottab-DUTY commented Oct 12, 2019

vTurbine commented Oct 12, 2019

vTurbine commented Oct 13, 2019 • edited Loading

SkyLoaderr commented Oct 14, 2019

SkyLoaderr Oct 14, 2019

Choose a reason for hiding this comment

vTurbine Oct 14, 2019

Choose a reason for hiding this comment

Xottab-DUTY Nov 1, 2019

Choose a reason for hiding this comment

Xottab-DUTY Nov 3, 2019 • edited Loading

Choose a reason for hiding this comment

007exe commented Oct 25, 2019

vTurbine commented Oct 26, 2019

Xottab-DUTY commented Oct 26, 2019 • edited Loading

Xottab-DUTY commented Oct 27, 2019

vTurbine commented Oct 27, 2019

Xottab-DUTY commented Oct 27, 2019

vTurbine commented Oct 31, 2019

Xottab-DUTY commented Oct 31, 2019

Xottab-DUTY left a comment

Choose a reason for hiding this comment

vTurbine commented Nov 4, 2019

Xottab-DUTY commented Nov 4, 2019

vTurbine commented Nov 4, 2019

Xottab-DUTY commented Nov 4, 2019

vTurbine commented Oct 13, 2019 •

edited

Loading

Xottab-DUTY Nov 3, 2019 •

edited

Loading

Xottab-DUTY commented Oct 26, 2019 •

edited

Loading