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

Code maintenance #165

Merged
merged 20 commits into from Nov 11, 2019
Merged

Code maintenance #165

merged 20 commits into from Nov 11, 2019

Conversation

@LovelySanta
Copy link
Collaborator

LovelySanta commented Oct 27, 2019

Describe the issue (if no issue has been made)

Code maintenance of things Cherno did start but not finish or mensioned before but did not do in the code maintenance video.

PR impact (Make sure to add closing keywords)

List of related issues/PRs this will solve:

Impact Issue/PR
Issues this solves Part 1 of #140
Other PRs this solves None

Proposed fix (Make sure you've read on how to contribute to Hazel)

A more detailed list can be found in #140 (comment)
  • Convert the remaining smart pointers to Hazel wrappers
    • std::shared_ptr -> Hazel::Ref and std::make_shared -> Hazel::CreateRef
    • std::unique_ptr -> Hazel::Scope and std::make_unique -> Hazel::CreateScope
  • Removing all DLL traces
    Removed all HAZEL_API macros
  • Fixing include statements
    • All includes relative to Hazel/src
    • All external includes use <> instead of ""
  • Use default (de)constructor instead of an empty one
  • Removed the dedicated (duplicate) BIND_EVENT_FN in application
    Now uses the generic HZ_BIND_EVENT_FN instead
  • Removed obsolete logging messages in glfw
  • Removed obsolete YAML keys in PR template

Additional context

Tested in debug and release, the rest of the code maintenace can be found in #140 (comment).

Puddlestomper and others added 12 commits Oct 17, 2019
also fix all uses of these methods accordingly
- LayerStack
- WindowCloseEvent
- AppTickEvent
- AppUpdateEvent
- AppRenderEvent
@Puddlestomper Puddlestomper mentioned this pull request Oct 27, 2019
30 of 42 tasks complete
Hazel/src/Hazel.h Show resolved Hide resolved
Hazel/src/Hazel/Core/Core.h Show resolved Hide resolved
Hazel/src/Hazel/Core/EntryPoint.h Outdated Show resolved Hide resolved
Sandbox/src/SandboxApp.cpp Outdated Show resolved Hide resolved
So the engine has full control over when the application is closed.
@CleverSource

This comment has been minimized.

Copy link
Contributor

CleverSource commented Oct 27, 2019

When will this be merged?

@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Oct 27, 2019

When will this be merged?

I think tomorrow, or after cherno's next vid/upload to github. However, I found some small things that still requires attention before I can merge it: #140 (comment)

@LovelySanta LovelySanta added this to Bugfixes (awaiting merge) in Community additions Oct 27, 2019
@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Oct 28, 2019

Should LovelySanta#47 be merged into this?

Suggested in comment
@CleverSource

This comment has been minimized.

Copy link
Contributor

CleverSource commented Oct 29, 2019

I don't think it should be merged into this.

@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Oct 30, 2019

I don't think it should be merged into this.

The reason I did it lije this, only Window.cpp includes the WindowsWindow, so in that file we can add/remove the obsolete classes when they are not required. This could even work for the different renderAPI's, for example exclude DirectX in macOS...

Any particular reason why you're against it @CleverSource ? I can see how it might require a seperate PR instead...

@CleverSource

This comment has been minimized.

Copy link
Contributor

CleverSource commented Oct 31, 2019

It's a good PR, but it doesn't feel very maintenance like and seeing as this PR is meant for maintenance I don't see why a PR unrelated should be merged with this.

While I do think the other PR should be implemented in some way, I don't think it should be added with this PR.

@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Nov 1, 2019

Removed LovelySanta#47 from the TODO list. I'm waiting till cherno pushes his code before I'll update this and merge it.

@CleverSource

This comment has been minimized.

Copy link
Contributor

CleverSource commented Nov 7, 2019

Will this be merged now?

@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Nov 7, 2019

Will this be merged now?

There are still some things to do, see #140, such as removing the OpenGL casting in the 3D renderer...

@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Nov 9, 2019

Will this be merged now?

Now everything is implemented. I want to have anther look in the weekend and I plan on merging it on monday if everything looks alright.

@LovelySanta LovelySanta self-assigned this Nov 9, 2019
Copy link
Contributor

Puddlestomper left a comment

Seems mostly good to merge. Only some minor things that we should clear up.

One other thing that I can think of that we might want to include, seeing as we have the platform detection now, is the include guards for the API-specific files depending on platform. The Windows* files would only be on Windows and the OpenGL* files can probably work on both Windows and Linux. Do you think we should do that? And, if so, should it be part of this PR?

Hazel/src/Hazel/Renderer/RendererAPI.cpp Show resolved Hide resolved
Sandbox/src/SandboxApp.cpp Outdated Show resolved Hide resolved
@LovelySanta

This comment has been minimized.

Copy link
Collaborator Author

LovelySanta commented Nov 10, 2019

One other thing that I can think of that we might want to include, seeing as we have the platform detection now, is the include guards for the API-specific files depending on platform. The Windows* files would only be on Windows and the OpenGL* files can probably work on both Windows and Linux. Do you think we should do that? And, if so, should it be part of this PR?

That is exactly what I asked some days ago: #165 (comment).

The only place Windows* files are included are within the Window and Input class for the WindowsWindow and WindowsInput respectively. By adding a create function to them (as done in LovelySanta#47), we can use the include guards to include the headers (if needed) and use them to return the correct implementation.

I agree with the reply from CleverSource #165 (comment) on the matter: It is a good PR, but not really maintenance, it should have its own PR into Hazel instead.

So I think this is indeed a nice suggestion, but also outside the scope of this PR?

@CleverSource

This comment has been minimized.

Copy link
Contributor

CleverSource commented Nov 10, 2019

It most likely is outside the scope of this PR to be quite honest.

@LovelySanta LovelySanta merged commit ccccfdc into TheCherno:master Nov 11, 2019
Community additions automation moved this from Bugfixes (awaiting merge) to Merged PR's Nov 11, 2019
AbdullahAmrSobh added a commit to AbdullahAmrSobh/Hazel that referenced this pull request Nov 11, 2019
- Convert smart pointers to Hazel wrappers: (Create)Scope and (Create)Ref
- Removing all DLL traces (removing all HAZEL_API macros)
- Fixing include statements:
      -> All includes relative to `Hazel/src`
      -> All external includes use `<>` instead of `""`
- Use default (de)constructor instead of an empty one
- Removed the dedicated (duplicate) `BIND_EVENT_FN` in application
- Removed obsolete logging messages in glfw
- Removed obsolete YAML keys in PR template
- Call Renderer2D::Shutdown() to free memory from allocated Renderer2DStorage
- Removed traces of OpenGL in the main Renderer and in Sandbox/ExampleLayer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Community additions
  
Merged PR's
4 participants
You can’t perform that action at this time.