Feedback on DirectX12 game engine structure, bad practices, and more | Together C & C++ | Page 1

drowsy swift Apr 11, 2023, 6:07 PM

#

I'm working on a game engine and it would be awesome with some feedback of overall structure, bad practices I'm doing, what I'm doing well, and maybe even some DirectX specific stuff. I understand that it's A LOT of code, but it would be the most helpful to get feedback on some of these classes: Engine, ResourceCache, Texture2DCache, VertexBufferCache, MaterialManager, DescriptorManager, ResourceEngine, ECS (and stuff in ECSBase.h), Scene, BufferResource (and it's subclasses), and TextureResource (and it's subclasses).
Here is the source code directory: https://github.com/NoahSch1999/aZeroEngine-Project/tree/master/aZeroEngine/aZeroEngine

GitHub

aZeroEngine-Project/aZeroEngine/aZeroEngine at master · NoahSch1999...

A game-engine I'm currently working on using DirectX12. - aZeroEngine-Project/aZeroEngine/aZeroEngine at master · NoahSch1999/aZeroEngine-Project

slim cargo Apr 11, 2023, 6:33 PM

#

Looks nice

#

I'll take a look at the code later

hard sedge Apr 11, 2023, 6:35 PM

#

drowsy swift I'm working on a game engine and it would be awesome with some feedback of overa...

First thing that catches the eye is you have a big folder with many files

#

it should be divided into sub folders

#

so it's easier to move through them

drowsy swift Apr 11, 2023, 6:37 PM

#

hard sedge First thing that catches the eye is you have a big folder with many files

ah yes. Will keep that in mind and fix it sometime in the future 🙂

hard sedge Apr 11, 2023, 6:37 PM

#

You also have no const correctness

#

no noexcept

#

why is scene a raw pointer?

#

and also why do you have so many shared pointers

#

why not just unique

drowsy swift Apr 11, 2023, 6:38 PM

#

hard sedge why is scene a raw pointer?

when?

drowsy swift Apr 11, 2023, 6:38 PM

#

hard sedge You also have no const correctness

i mightve forgotten const correctness in some classes then. cause a lot of them has it. look at the resourcecache subclasses 🙂

hard sedge Apr 11, 2023, 6:39 PM

#

drowsy swift when?

graphics

drowsy swift Apr 11, 2023, 6:39 PM

#

hard sedge no noexcept

but yes, that i should add more, thanks 🙂

drowsy swift Apr 11, 2023, 6:39 PM

#

hard sedge graphics

Oh, sry. Graphics and EditorUI are depricated!

#

should edit that!

hard sedge Apr 11, 2023, 6:39 PM

#

its also a good practice to use C++ style cast instead of C style cast

#

since C style one will iterate over casts and in most cases it doesnt matter but once it does u will have quite a debugging time

drowsy swift Apr 11, 2023, 6:39 PM

#

hard sedge its also a good practice to use C++ style cast instead of C style cast

ye. gonna add that as well. thanks for the reminder!

#

@hard sedge

hard sedge Apr 11, 2023, 6:41 PM

#

just remove the deprecated code/files

#

why would it lay in the repo

drowsy swift Apr 11, 2023, 6:41 PM

#

hard sedge why would it lay in the repo

good point!

hard sedge Apr 11, 2023, 6:42 PM

#

Also if you're using imgui its good to abstract some widget class that will encapsulate it

#

so it's less code to use it and you have an easier control

#

it's cleaner

#

if (ImGui::Begin("Global Lighting Manager"))
    {

        ImGui::Text("Directional Light");
        static float pitch = 0.f;
        static float yaw = 0.f;
        static float roll = 0.f;
        ImGui::SliderAngle("Pitch", &pitch);
        ImGui::SliderAngle("Yaw", &yaw);

        Matrix view = Matrix::CreateFromYawPitchRoll(yaw, pitch, roll);
        view = Matrix::CreateLookAt(Vector3(0, 0, 0), Vector3(0, 0, 1), Vector3(0, 1, 0));
        Matrix proj = Matrix::CreateOrthographic(10.f, 10.f, 1.f, 10.f);
        graphics.lightSystem->GetLightManager()->dLightData.VPMatrix = proj * view;

        ImGui::End();
    }```

#

ah ok its deprecated nvm

drowsy swift Apr 11, 2023, 6:43 PM

#

The new UI is LevelEditorUI

#

I should change the scene inside UI to be unique_ptr btw

hard sedge Apr 11, 2023, 6:44 PM

#

if (ImGui::IsKeyPressed(ImGuiKey_W))
                mCurrentGizmoOperation = ImGuizmo::TRANSLATE;
            if (ImGui::IsKeyPressed(ImGuiKey_R))
                mCurrentGizmoOperation = ImGuizmo::ROTATE;
            if (ImGui::IsKeyPressed(ImGuiKey_E))
                mCurrentGizmoOperation = ImGuizmo::SCALE;```

#

I dont like this

#

YOu have a separate class for input

#

but you're adding another checks here

#

You should have only one object that is handling the input

drowsy swift Apr 11, 2023, 6:45 PM

#

hard sedge but you're adding another checks here

Oh yeah. thanks! will change that

hard sedge Apr 11, 2023, 6:45 PM

#

not using it in N places

drowsy swift Apr 11, 2023, 6:45 PM

#

yeah, totally missed that.

hard sedge Apr 11, 2023, 6:45 PM

#

Transform* tf = currentScene->GetComponentForEntity<Transform>(_entity);```

#

also naming

#

tf is a unreadable name

#

or cam

#

use proper names since its just hard to read

#

also you use a lot of shared ptrs in a lot of places where unique_ptr will suffice

#

its just once you get wrong references somewhere its hard to debug

#

and its unnecessary fight

#

you also have quite a number of unnecessary static variables

#

they could just be placed on stack in scope and everything should be fine

drowsy swift Apr 11, 2023, 6:48 PM

#

hard sedge they could just be placed on stack in scope and everything should be fine

which one specifically?

hard sedge Apr 11, 2023, 6:48 PM

#

like this cpp static int counter = 0; in level editor ui

#

It just created a lot of possible problems

#

static char matNameBuffer[64] = "";```

#

also this I hate

#

just use std::string and resize it

drowsy swift Apr 11, 2023, 6:49 PM

#

hard sedge like this ```cpp static int counter = 0;``` in level editor ui

ah that is uneccessary. will remove counter fully

hard sedge Apr 11, 2023, 6:49 PM

#

and keep it as member variable

#

static bool alrExists = false;

#

same here

#

then - I dont like that the editor class is doing a lot of stuff like loading/saving/deleting

#

It should have the widgets abstracted and just draw/update widgets

drowsy swift Apr 11, 2023, 6:50 PM

#

hm. i feel like it isnt necessary to have the class hold it. alrExists is only used for that specific area

hard sedge Apr 11, 2023, 6:50 PM

#

and the widgets itself should handle all that stuff

#

(char*)&info.normalMapIndex

#

this is also bad just use the C++ cast everywhere

#

virtual void Update() override
    {
        // Prep pipeline
        if (!camera.expired())
        {
            rtv->Clear(resourceEngine->renderPassList);
            dsv->Clear(resourceEngine->renderPassList);
            resourceEngine->renderPassList.GetGraphicList()->OMSetRenderTargets(1, &rtv->GetHandle().GetCPUHandleRef(), true, &dsv->GetHandle().GetCPUHandleRef());
            resourceEngine->renderPassList.GetGraphicList()->IASetPrimitiveTopology(D3D12_PRIMITIVE_TOPOLOGY::D3D_PRIMITIVE_TOPOLOGY_TRIANGLELIST);
            resourceEngine->renderPassList.GetGraphicList()->RSSetScissorRects(1, &swapChain->GetScissorRect());
            resourceEngine->renderPassList.GetGraphicList()->RSSetViewports(1, &swapChain->GetViewPort());

            resourceEngine->renderPassList.GetGraphicList()->SetPipelineState(pso.GetPipelineState());
            resourceEngine->renderPassList.GetGraphicList()->SetGraphicsRootSignature(rootSignature.GetSignature());

            std::shared_ptr<Camera> cam = camera.lock();
            resourceEngine->renderPassList.GetGraphicList()->SetGraphicsRootConstantBufferView(1, cam->GetBuffer().GetGPUAddress());

            for (Entity ent : entityIDMap.GetObjects())
            {
                resourceEngine->renderPassList.GetGraphicList()->SetGraphicsRootConstantBufferView(0, componentManager.GetComponent<Transform>(ent)->GetBuffer().GetGPUAddress());
                resourceEngine->renderPassList.GetGraphicList()->SetGraphicsRoot32BitConstant(2, ent.id, 0);
                VertexBuffer* vb = vbCache->GetResource(componentManager.GetComponent<Mesh>(ent)->GetID());
                resourceEngine->renderPassList.GetGraphicList()->IASetVertexBuffers(0, 1, &vb->GetView());
                resourceEngine->renderPassList.GetGraphicList()->DrawInstanced(vb->GetNumVertices(), 1, 0, 0);
            }

            resourceEngine->RequestReadback(rtv);
        }
    }```

#

why in the world a class related to picking objects would have such update

drowsy swift Apr 11, 2023, 6:53 PM

#

hard sedge why in the world a class related to picking objects would have such update

its a system which handles picking for objects. it needs to render the objects to be able to use for picking

hard sedge Apr 11, 2023, 6:53 PM

#

just store the entities in the scene/level and get the array here via some getter on an event based approach once we want to pick smth

drowsy swift Apr 11, 2023, 6:55 PM

#

hard sedge just store the entities in the scene/level and get the array here via some gette...

all entities might not be bound to that system thought. then i would have to look for all entities which should be pickable.

hard sedge Apr 11, 2023, 6:56 PM

#

You can just have an hash map on each level and iterate through every level

#

or layer

#

or anything that you have

#

and do a simple lockup

#

only once you want to pick smth

#

I see no point in using such update

#

waste of cycles and makes the architecture worse to use

drowsy swift Apr 11, 2023, 6:57 PM

#

but i need to draw the models to a texture in order to read back the entity id

#

what i really should be doing is merging that draw into the geometry pass instead

hard sedge Apr 11, 2023, 6:57 PM

#

Then you could abstract the renderer into an array of render passess

drowsy swift Apr 11, 2023, 6:57 PM

#

and just output to this rtv aswell

hard sedge Apr 11, 2023, 6:57 PM

#

add an ID render pass

#

and just read it here

drowsy swift Apr 11, 2023, 6:58 PM

#

yeah. i think that would be the best approuch

hard sedge Apr 11, 2023, 6:58 PM

#

only one something is supposed to be selected

#

since you dont need to render ID every time

#

only if clicked

drowsy swift Apr 11, 2023, 6:58 PM

#

as i wrote in the description. this class isnt really what i need feedback from atm since its not very polished. thats why its not in the list.

hard sedge Apr 11, 2023, 6:59 PM

#

static bool minimized = false;
    static bool paused = false;```

#

also again

#

you instead of using members/local vars create statics

drowsy swift Apr 11, 2023, 6:59 PM

#

i'll remove this later and output the geometry data and entity id to an extra rendertarget during the geometry pass, which i then can sample from on the cpu. this removes an extra draw call per entity

hard sedge Apr 11, 2023, 6:59 PM

#

that may make your life way harder

#

for no reaosn

drowsy swift Apr 11, 2023, 7:00 PM

#

hard sedge ```cpp static bool minimized = false; static bool paused = false;```

where is that?

hard sedge Apr 11, 2023, 7:00 PM

#

appwindow

drowsy swift Apr 11, 2023, 7:00 PM

#

well. those arent used. ive totally missed those lol xd

hard sedge Apr 11, 2023, 7:00 PM

#

then this

#

#define FORWARD Vector3(0,0,1)
    #define VECRIGHT Vector3(1,0,0)
    #define UP Vector3(0,1,0)```

#

those should actually be statics

drowsy swift Apr 11, 2023, 7:01 PM

#

as i said, the classes in the description are the ones most polished and best to give feedback on, since they wont have a bunch of non-used code

hard sedge Apr 11, 2023, 7:02 PM

#

wheres the description

#

I dont see it

drowsy swift Apr 11, 2023, 7:02 PM

#

hard sedge Apr 11, 2023, 7:05 PM

#

int Texture2DCache::GetTextureHeapID(int _key)
{
Texture2D* text = resources.GetObjectByKey(_key);
if (text)
return text->GetHandle().GetHeapIndex();

return -1;

}

#

the text should be const for example

#

the method marked as const

#

and as NODISCARD

#

its a good practice to give more info to other programmers and those keywords are great to do that

#

Texture2D* t = resources.GetObjectByKey(_key);

#

again naming

#

and no const

drowsy swift Apr 11, 2023, 7:08 PM

#

Ah totally forgot about const cor when it comes to variables and not functions xD ye. Gotta make a lot of changes to that

hard sedge Apr 11, 2023, 7:09 PM

#

std::vector<std::shared_ptr<TextureResource>>

#

this is asking for problems

#

DescriptorManager& GetDescriptorManager() { return descriptorManager; }

#

and this should return a const DescriptorManager&

#

if you dont want someone to modify but just get a variable you add a const to the type

#

if its not by copy

#

In summary name things properly, add const wherever you can, do things on event based approach, abstract stuff so its cleaner and easier to use

#

and make it somewhat solid

#

so that the class only does its job and nothing else

#

ok also this

#

namespace COMPONENTENUM
{
    enum COMPONENTBITID { TRANSFORM, MESH, MATERIAL, PLIGHT, DLIGHT, MAX };
    inline std::string COMPONENTNAMES[4]{ "Transform", "Mesh", "Material", "Point Light" };
}```

#

just create some component base class inherit from it and add it to the entities

#

have an array of components

#

why hardcoding name here

#

what if someone creates a new one

#

he has to move to this file and hardcode 2 values

#

its bad

#

then you have multiple classess in this file

#

its not good at all

#

harder to read/go through

#

struct Entity
{
    Entity() = default;
    int id = -1; /**<Unique ID mapped to a component within the ComponentManager class that the component was registered for using ComponentManager::RegisterComponent(Entity& _entity, const T& _initValue)*/
    std::bitset<MAXCOMPONENTS> componentMask; /**<Describes what type of components that the instance of the Entity has registered*/
};```

#

this is a big no no

#

it should just have an vector of components

#

and it should describe the main entity

#

that later u can inherit from like to create a widget, scene entities and so on

#

then I dont want to go though this whole file

#

it just looks like a mess - no offense

#

a lot of things happening in a file called ECSBase.h

drowsy swift Apr 11, 2023, 7:24 PM

#

hard sedge std::vector<std::shared_ptr<TextureResource>>

not rly. it needs them to stay alive.

drowsy swift Apr 11, 2023, 7:25 PM

#

hard sedge have an array of components

i need to separate my component types in different arrays

hard sedge Apr 11, 2023, 7:26 PM

#

drowsy swift i need to separate my component types in different arrays

sure you can separate them by child classess

#

not by hardcoding values somewhere

drowsy swift Apr 11, 2023, 7:26 PM

#

hard sedge what if someone creates a new one

no one is supposed to create new ones. me, the engine dev, is doing that

hard sedge Apr 11, 2023, 7:26 PM

#

it makes it way worse to use

#

even for you

#

after some time u still need to remember to go to a file and add values

#

and why

#

just use simple inheritance

#

and abstraction

#

to make it dynamic

#

and u wont need to modify some values somewhere

drowsy swift Apr 11, 2023, 7:26 PM

#

ait. will look into that

hard sedge Apr 11, 2023, 7:27 PM

#

drowsy swift not rly. it needs them to stay alive.

then just use unique ptrs in objects that own them

#

and properly remove them from this array on event based approach once they die

drowsy swift Apr 11, 2023, 7:28 PM

#

hard sedge then just use unique ptrs in objects that own them

no. i need the resource to stay alive even if the requested resource goes out of scope.

#

by calling ResourceEngine::RequestReadback() i can request the resource engine to readback data in a gpu/cpu safe way. What if the input resource goes out of scope before the gpu-commands are executed/while they are executed?

#

that will cause UB/crash

hard sedge Apr 11, 2023, 7:30 PM

#

just abstract the commands

#

you overthink a simple problem

#

add a boolean that will tell if they're during execution

#

once all finished

#

remove the resource

drowsy swift Apr 11, 2023, 7:30 PM

#

and how do i tell that they are executing?

hard sedge Apr 11, 2023, 7:31 PM

#

synchronization techniques like fence

drowsy swift Apr 11, 2023, 7:31 PM

#

that would also mean that i would have to traverse between all resources and check (or something similar atleast, no)?

hard sedge Apr 11, 2023, 7:31 PM

#

you have fence already implemented in dx12

#

no lol

drowsy swift Apr 11, 2023, 7:32 PM

#

hard sedge no lol

why?

hard sedge Apr 11, 2023, 7:33 PM

#

ID3D12Fence* fence;
UINT64 fenceValue = 1;
HRESULT hr = device->CreateFence(0, D3D12_FENCE_FLAG_NONE, IID_PPV_ARGS(&fence));
//...

HANDLE fenceEvent = CreateEvent(nullptr, FALSE, FALSE, nullptr);

UINT64 currentFenceValue = fenceValue++;
commandQueue->Signal(fence, currentFenceValue);

//...

if (fence->GetCompletedValue() < currentFenceValue)
{
    fence->SetEventOnCompletion(currentFenceValue, fenceEvent);
    WaitForSingleObject(fenceEvent, INFINITE);
}

drowsy swift Apr 11, 2023, 7:33 PM

#

that queues a cpu side wait

#

i dont wanna do that every frame lol

hard sedge Apr 11, 2023, 7:33 PM

#

bro just again

hard sedge Apr 11, 2023, 7:33 PM

#

drowsy swift that would also mean that i would have to traverse between all resources and che...

event based

#

removal

#

once you want to remove something

#

you test it with the abstraction

hard sedge Apr 11, 2023, 7:34 PM

#

drowsy swift that queues a cpu side wait

that will make your architecture clean/dynamic and simple to use

#

and will cost nothing since ur bottleneck is on gpu

drowsy swift Apr 11, 2023, 7:34 PM

#

thats such a broad word. could you be more specific?

drowsy swift Apr 11, 2023, 7:34 PM

#

hard sedge and will cost nothing since ur bottleneck is on gpu

not rly. depends on the gpu work, the cpu specs, and a lot more

hard sedge Apr 11, 2023, 7:35 PM

#

... xd

#

ok w/e

#

no it doesnt but have it your way

#

anyway gl

drowsy swift Apr 11, 2023, 7:35 PM

#

lol. why so toxic? Instead of just saying that something is bad, explain it so I understand the reason that it is bad

hard sedge Apr 11, 2023, 7:36 PM

#

your app does little to nothing on cpu

#

so all bottleneck is on gpu

#

its just obvious

#

by using shared pointers you just create a lot of ways to make it static/hard to use/with a lot of random bugs

#

whereas you can abstract it, use fences and what not

drowsy swift Apr 11, 2023, 7:36 PM

#

thats what i dont get. later when i add collision, physics, etcetc it will tax the cpu more

hard sedge Apr 11, 2023, 7:36 PM

#

and just queue a deletion once something dies

#

you just run it on separate thread and the fences will still cost nothing

#

to later synchroniza everything

drowsy swift Apr 11, 2023, 7:38 PM

#

hard sedge and just queue a deletion once something dies

how would that deletion look like?

hard sedge Apr 11, 2023, 7:39 PM

#

one of many ways

#

have an array of unique ptrs

#

have the resource know the holder id or whatnot

#

once the object with certain id is destroyed

#

queue destruction of certain resource

drowsy swift Apr 11, 2023, 7:40 PM

#

so all resources are in unique ptrs?

hard sedge Apr 11, 2023, 7:40 PM

#

yes

drowsy swift Apr 11, 2023, 7:40 PM

#

thats gonna cause insane fragmentation

hard sedge Apr 11, 2023, 7:41 PM

#

nah

drowsy swift Apr 11, 2023, 7:42 PM

#

why? say i have 1000 transform constant buffers. they all are in unique_ptrs. i iterate through all transform components in some system. then i have to go all over the memory

hard sedge Apr 11, 2023, 7:42 PM

#

you know whats the problem with C++ and C programmers once they start?

drowsy swift Apr 11, 2023, 7:43 PM

#

"they overoptimize when its not neccessary"

hard sedge Apr 11, 2023, 7:43 PM

#

They think that saving some cycles each frame is a great must and will make the architecture insanely bad just fo the sake of premature optimizations

#

ur cpu

#

wont give a single

#

fuck

#

about 1k objects

drowsy swift Apr 11, 2023, 7:44 PM

#

but its just a fact that having all resources in pointers instead of contiguous in a vector is much slower and scales a lot worse. so why start doing something in a mediocre way?

hard sedge Apr 11, 2023, 7:45 PM

#

because it doesnt matter and you have shared pointers anyway

drowsy swift Apr 11, 2023, 7:45 PM

#

however, there's probably a better way to queue the resources to be removed without having to store the resources in pointers

drowsy swift Apr 11, 2023, 7:46 PM

#

hard sedge because it doesnt matter and you have shared pointers anyway

i dont?

hard sedge Apr 11, 2023, 7:46 PM

#

std::vector<std::shared_ptr<TextureResource>>

drowsy swift Apr 11, 2023, 7:47 PM

#

ah okay. then theres a missunderstanding. those arent components

#

mb

#

However, that method is just to read from arbitrary textureresources.

#

resourceEngine->RequestReadback(rtv);

drowsy swift Apr 11, 2023, 7:50 PM

#

hard sedge queue destruction of certain resource

i will look into some kind of version like this thought. that is good for non-components.

#Feedback on DirectX12 game engine structure, bad practices, and more