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

Make more use of DefineConsoleMethod #1072

Merged
merged 11 commits into from Dec 26, 2014

Conversation

Projects
None yet
5 participants
@crabmusket
Contributor

crabmusket commented Dec 21, 2014

Resubmission of #887 with some fixes.

@crabmusket crabmusket added the Defect label Dec 21, 2014

@crabmusket crabmusket added this to the 3.7 milestone Dec 21, 2014

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

As general note... if (dStrcmp(stringblabla,"") != 0) better use dStrIsEmpty for more clean to read.

Contributor

LuisAntonRebollo commented Dec 23, 2014

As general note... if (dStrcmp(stringblabla,"") != 0) better use dStrIsEmpty for more clean to read.

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

Another general note. See this:

- Con::printf("%s(): Invalid SimObject: %s", (const char*)argv[0], (const char*)argv[2]);
+ Con::printf("%s(): Invalid SimObject: %s", object->getName(), objName);

I dont like this type of change becouse not all object have name, and we can loose a lot of info.

Better?
Con::printf("CLASS::FUNCTION() [%s]: Invalid SimObject: %s", object->getName(), objName);

Contributor

LuisAntonRebollo commented Dec 23, 2014

Another general note. See this:

- Con::printf("%s(): Invalid SimObject: %s", (const char*)argv[0], (const char*)argv[2]);
+ Con::printf("%s(): Invalid SimObject: %s", object->getName(), objName);

I dont like this type of change becouse not all object have name, and we can loose a lot of info.

Better?
Con::printf("CLASS::FUNCTION() [%s]: Invalid SimObject: %s", object->getName(), objName);

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

We need to review some TABs in code..... no more TABS comments :P

Contributor

LuisAntonRebollo commented Dec 23, 2014

We need to review some TABs in code..... no more TABS comments :P

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

We need to remove Winterleaf mentions from code, no?

Contributor

LuisAntonRebollo commented Dec 23, 2014

We need to remove Winterleaf mentions from code, no?

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

We need to remove old commented code.

Contributor

LuisAntonRebollo commented Dec 23, 2014

We need to remove old commented code.

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Dec 23, 2014

Luis: Thanks for taking the time to do a thorough review of this PR so that I didn't have to. 👍

dottools commented Dec 23, 2014

Luis: Thanks for taking the time to do a thorough review of this PR so that I didn't have to. 👍

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

mmmm too tired for continue :(

Need to continue tomorrow on Engine/source/sfx/sfxSystem.cpp

Contributor

LuisAntonRebollo commented Dec 23, 2014

mmmm too tired for continue :(

Need to continue tomorrow on Engine/source/sfx/sfxSystem.cpp

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

@Winterleaf, a lot of work in this PRs... thx guys :)

@dottools, wow much more work than spected :P

Contributor

LuisAntonRebollo commented Dec 23, 2014

@Winterleaf, a lot of work in this PRs... thx guys :)

@dottools, wow much more work than spected :P

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Dec 23, 2014

Contributor

@LuisAntonRebollo whoah, I should learn from you. Great thorough review. I'll start to make these changes. I will confess I got lazy and didn't use dStrIsEmpty in a lot of places I could have :P.

Contributor

crabmusket commented Dec 23, 2014

@LuisAntonRebollo whoah, I should learn from you. Great thorough review. I'll start to make these changes. I will confess I got lazy and didn't use dStrIsEmpty in a lot of places I could have :P.

Show outdated Hide outdated Engine/source/util/settings.cpp Outdated

crabmusket added some commits Dec 23, 2014

Fixes after feedback from Luis.
 * Made use of dStrIsEmpty in more locations (and fixed it :P)
 * Removed commented-out code
 * Corrected default params
 * Fixed some console warning formats
 * Removed tabs
 * Corrected setExtent API
Revert changes to uses of setExtent.
This reverts part of commit 9907c45.
@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

@eightyeight , thx for all quick fixes :)

I'm going to check we are not lossing something and later continue with un-reviewed part.

Contributor

LuisAntonRebollo commented Dec 23, 2014

@eightyeight , thx for all quick fixes :)

I'm going to check we are not lossing something and later continue with un-reviewed part.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Dec 23, 2014

Contributor

I reviewed the whole thing, but feel free to double-check :). In my changes I also reverted Vince's change to setExtent, which broke the API. I restored the original API and reverted all the script changes which were made to accommodate it.

Contributor

crabmusket commented Dec 23, 2014

I reviewed the whole thing, but feel free to double-check :). In my changes I also reverted Vince's change to setExtent, which broke the API. I restored the original API and reverted all the script changes which were made to accommodate it.

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

Removed a lot of outdated comments fixed by @eightyeight ... thx :)

Contributor

LuisAntonRebollo commented Dec 23, 2014

Removed a lot of outdated comments fixed by @eightyeight ... thx :)

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Dec 23, 2014

Contributor

Why about preserve old functions for sfxSystem.cpp for avoid ugly port of SFXTrack track | ( SFXDescription description, string filename ) ?

Contributor

LuisAntonRebollo commented Dec 23, 2014

Why about preserve old functions for sfxSystem.cpp for avoid ugly port of SFXTrack track | ( SFXDescription description, string filename ) ?

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Dec 23, 2014

Contributor

I think it's fine to just focus on porting the use of ConsoleMethod to DefineConsoleMethod, and worry about the niceness later :/.

Contributor

crabmusket commented Dec 23, 2014

I think it's fine to just focus on porting the use of ConsoleMethod to DefineConsoleMethod, and worry about the niceness later :/.

Merge branch 'development' into defineconsolemethod
Conflicts:
	Engine/source/materials/materialDefinition.cpp
@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Dec 26, 2014

Contributor

Merge build was aborted due to CI screwup. My bad. This should be good to go, and we'll see if it causes any more issues.

Contributor

crabmusket commented Dec 26, 2014

Merge build was aborted due to CI screwup. My bad. This should be good to go, and we'll see if it causes any more issues.

crabmusket added a commit that referenced this pull request Dec 26, 2014

Merge pull request #1072 from eightyeight/defineconsolemethod
Make more use of DefineConsoleMethod

@crabmusket crabmusket merged commit 59a5261 into GarageGames:development Dec 26, 2014

1 check failed

default Merged build finished.
Details

@crabmusket crabmusket deleted the crabmusket:defineconsolemethod branch Dec 26, 2014

crabmusket referenced this pull request in crabmusket/Torque3D Jan 2, 2015

@Azaezel Azaezel referenced this pull request Mar 13, 2015

Closed

fullscreen alt+tab erorrs #1250

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