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

AmberScript updates to buffers #66

Merged
merged 4 commits into from
Nov 20, 2018
Merged

AmberScript updates to buffers #66

merged 4 commits into from
Nov 20, 2018

Conversation

dj2
Copy link
Collaborator

@dj2 dj2 commented Nov 19, 2018

No description provided.

@dj2 dj2 added the documentation Issues with documentation label Nov 19, 2018
@dj2 dj2 added this to the AmberScript Parsing milestone Nov 19, 2018
@dj2 dj2 self-assigned this Nov 19, 2018
@dj2 dj2 added this to In progress in General via automation Nov 19, 2018
@dj2 dj2 requested review from jaebaek and dneto0 November 19, 2018 18:36
docs/amber_script.md Show resolved Hide resolved
ENTRY_POINT kFragmentShader red
END  # pipeline

PIPELINE graphics kGreenPipeline
ATTACH kVertexShader
ATTACH kFragmentShader

FRAMEBUFFER kFrameBuffer
FRAMEBUFFER 256 256
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we encode the case where we want the framebuffer from the two pipelines to be the same, or to be different? My previous assumption was they'd have unique framebuffers, except in cases like this were I was passing the same FRAMEBUFFER into both of them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

   BIND myim AS fb DESCRIPTOR_SET 1 BINDING 0

Then the underlying resource (myim) is a declared buffer (image) that already exists. And you can act on it twice, and it's orthogonal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@@ -405,6 +403,6 @@ CLEAR_COLOR kGraphicsPipeline 255 0 0 255
CLEAR kGraphicsPipeline

RUN kGraphicsPipeline \
 DRAW_ARRAY kIndices IN kBuffer AS triangle_list \
 DRAW_ARRAY kIndices IN kData AS triangle_list \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently RUN is where we use vertex and indices buffers, do they need to be attached into a PIPELINE with the VERTEX_BUFFER and INDEX_BUFFER calls we mentioned, or is just referencing them here enough?

If they're attached into a PIPELINE, does that mean that RUN pipeline DRAW_ARRAY will just use them implicitly? So this would just be:

 RUN kGraphicsPipeline DRAW_ARRAY AS triangle_list START_IDX 0 COUNT 24

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Vulkan both index buffer and vertex buffers are bound in the command buffer before doing the draw call. So it's ok to do it implicitly here. The backend will translate this RUN call into the right bind-index-buffer and bind-vertex-buffer commands.

I believe the vertex and index buffer bindings are totally different numbering spaces than descriptor set/binding pairs. Beware.(?!)

docs/amber_script.md Show resolved Hide resolved
docs/amber_script.md Show resolved Hide resolved
docs/amber_script.md Show resolved Hide resolved
ENTRY_POINT kFragmentShader red
END  # pipeline

PIPELINE graphics kGreenPipeline
ATTACH kVertexShader
ATTACH kFragmentShader

FRAMEBUFFER kFrameBuffer
FRAMEBUFFER 256 256
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example:

   BIND myim AS fb DESCRIPTOR_SET 1 BINDING 0

Then the underlying resource (myim) is a declared buffer (image) that already exists. And you can act on it twice, and it's orthogonal.

@@ -405,6 +403,6 @@ CLEAR_COLOR kGraphicsPipeline 255 0 0 255
CLEAR kGraphicsPipeline

RUN kGraphicsPipeline \
 DRAW_ARRAY kIndices IN kBuffer AS triangle_list \
 DRAW_ARRAY kIndices IN kData AS triangle_list \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Vulkan both index buffer and vertex buffers are bound in the command buffer before doing the draw call. So it's ok to do it implicitly here. The backend will translate this RUN call into the right bind-index-buffer and bind-vertex-buffer commands.

I believe the vertex and index buffer bindings are totally different numbering spaces than descriptor set/binding pairs. Beware.(?!)

@dj2
Copy link
Collaborator Author

dj2 commented Nov 19, 2018

PTAL.

I decided to run with VERTEX_DATA and INDEX_DATA to see what it looked it, and seems to make the DRAW_ARRAY call nicer. This then does raise the question, can you attach multiple vertex data buffers to a pipeline? If you want to have multiple data types in there you'd need to, or we need struct support for the data types.

Changed up the FRAMEBUFFER settings a bit to match what was discussed.

BIND BUFFER <buffer_name> AS <buffer_type> DESCRIPTOR_SET <id> \
BINDING <id> IDX <val>

BIND SAMPLER <sampler_name>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Samplers are also bound to descriptor set and binding.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

### Run a pipeline.

When running a `DRAW_ARRAY` command you must attach the vertex data and index
data to the `PIPELINE` with the `VERTEX_DATA` and `INDEX_DATA` commands.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The index buffer is only needed if doing an indexed draw. (It's if and only if.)

I'd write it like this:

When running a DRAW_ARRAY command, you must attache the vertex data to the PIPELINE with the VERTEX_DATA command.
To run an indexed draw, attach the index data to the PIPELINE with an INDEX_DATA command.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also updated DRAW_ARRAY to provide a DRAW_ARRAY INDEXED option.

docs/amber_script.md Show resolved Hide resolved
@@ -319,15 +338,15 @@ PIPELINE graphics kRedPipeline

ATTACH kFragmentShader

FRAMEBUFFER kFrameBuffer
FRAMEBUFFER kFramebuffer 256 256
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need DIMS keyword

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

docs/amber_script.md Show resolved Hide resolved
VERTEX_DATA kData
INDEX_DATA kIndices
END  # pipeline

CLEAR_COLOR kGraphicsPipeline 255 0 0 255
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that FRAMEBUFFER is not declared in a pipeline, the CLEAR and CLEAR_COLOR have to name an buffer rather than a pipeline, I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you're still clearing the drawing buffer for the pipeline aren't you as opposed to some other buffer?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's resolve Framebuffer semantics in a future CL.

docs/amber_script.md Show resolved Hide resolved
VERTEX_DATA kData
INDEX_DATA kIndices
END  # pipeline

CLEAR_COLOR kGraphicsPipeline 255 0 0 255
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's resolve Framebuffer semantics in a future CL.

@dneto0 dneto0 merged commit 3d0472b into google:master Nov 20, 2018
General automation moved this from In progress to Done Nov 20, 2018
@dj2 dj2 mentioned this pull request Nov 21, 2018
@dj2 dj2 deleted the buffer_updates branch December 5, 2018 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues with documentation
Projects
No open projects
General
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants