webai-community / aquarium

BSD 3-Clause "New" or "Revised" License
26 stars 7 forks source link

Prevent potential combination explosion of backend types #165

Closed hujiajie closed 4 years ago

gyagp commented 4 years ago

@shaoboyan @Jiawei-Shao Could you please provide some comments here?

hujiajie commented 4 years ago

Now I'm feeling we should decide on several orthogonal issues. Let me echo the biggest two here:

1) Shall we use a combination of a) backend of Aquarium (ContextGL / ContextDawn / ContextD3D12) + (optional) native backend of a Aquarium backend, or b) (optional) translation layer (ANGLE / Dawn) + native backend?

2) Shall we use std::pair<> or bitfields to represent the backend?

shaoboyan commented 4 years ago

After some discussion with Jiajie, I think I support Jiajie's first a) option. It seems will have a better code shape. My feeling is that I'm not confortable for the shape like {BACKENDTYPE::BACKENDTYPEOPENGL, BACKENDTYPE::BACKENDTYPELAST}; They're in different filed but with the same type name. So I tend to choose bitfiled and high/low bit for this or the different enum types with std::pair.

hujiajie commented 4 years ago

Thanks, @shaoboyan. To clarify, the shape of (1.a) is:

switch (aquariumRenderer) {
case OPENGL:
case ANGLE:
  ctx = new ContextGL(rendererBackend); break;
case DAWN:
  ctx = new ContextDawn(rendererBackend); break;
case D3D12:
  ctx = new ContextD3D12(rendererBackend); break;
}

The shape of (1.b) is:

if ((translationLayer == NONE && nativeBackend == OPENGL) || translationLayer == ANGLE) {
  ctx = new ContextGL(translationLayer, nativeBackend);
} else if (translationLayer == DAWN) {
  ctx = new ContextDawn(translationLayer, nativeBackend);
} else if (translationLayer == NONE && nativeBackend == D3D12) {
  ctx = new ContextD3D12(translationLayer, nativeBackend);
}
shaoboyan commented 4 years ago

After some further discussion with Yang and Jiajie, I support Yang's suggestion here https://github.com/webatintel/aquarium/pull/165#discussion_r495534105 if and only if Jiajie accept the bigger suggestion that we should take renderer as an important design concept and do the changes.

hujiajie commented 4 years ago

To summarize the vote, the combination in (1.b) + bitfields is a little more popular till the moment, and continuous numbering was rejected.