Fix to include a needed include for the accumulation volume stuffs #1725

Merged
merged 1 commit into from Aug 7, 2016

Conversation

Projects
None yet
2 participants
@Areloch
Contributor

Areloch commented Aug 7, 2016

Fix to include a needed include for the accumulation volume stuffs

@Areloch Areloch added the Bugfix label Aug 7, 2016

@Areloch Areloch merged commit dfa6d46 into GarageGames:development Aug 7, 2016

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Aug 7, 2016

The only problem I see with this change is that the include used relative to source file path instead of engine source full path to header file.
Meaning it should have been this:

#include "T3D/accumulationVolume.h"

Because Engine/source is the top-level include directive to the compiler for the engine executable/library project.

Includes aren't suppose to be using relative to source file paths on purpose so that everybody knows where exactly the header file is located, and avoids possible conflicts if there are multiple header files of the same name in include path directives. Now there are exceptions for things like library includes and what not where it cannot be helped, but if it's apart of the same project directory structure then it should use source directory full path to header file as shown above.

dottools commented Aug 7, 2016

The only problem I see with this change is that the include used relative to source file path instead of engine source full path to header file.
Meaning it should have been this:

#include "T3D/accumulationVolume.h"

Because Engine/source is the top-level include directive to the compiler for the engine executable/library project.

Includes aren't suppose to be using relative to source file paths on purpose so that everybody knows where exactly the header file is located, and avoids possible conflicts if there are multiple header files of the same name in include path directives. Now there are exceptions for things like library includes and what not where it cannot be helped, but if it's apart of the same project directory structure then it should use source directory full path to header file as shown above.

@John3 John3 referenced this pull request Aug 7, 2016

Merged

added path @dottools #1726

@Areloch Areloch deleted the Areloch:IncludeInclude branch Jul 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment