Re: OD - Ordered TODO list thread

Re: OD - Ordered TODO list thread

Postby Bertram » 03 Jul 2014, 16:14

EDIT: N.B: Obsolete topic. :3 Feel free to have a look here: https://github.com/OpenDungeons/OpenDun ... ones/0.4.9


Hi again, (I'm spamming today ;])

@Paul
Well , origin/ paul424-integration right ?


First of all, nice work done there. :) There are a few things but I can feel the beast growing nicely. An extra point for certain commits fixing indentation and adding headers, I appreciate it.

Now for the requested fixes before merging:

- There are broken indentations in SlopWalk.cpp.
- Please use only spaces, not tabs in the gui files, such as the looknfeel one, to be in sync with what's done in the source code.
- Could you rename the Channel* console commands to something likr Debug* or DbgChannel* since it's more meaningful to me? (What do you think?)
- You've added ${SRC}/ChannelDebug.cpp twice in the CMakeLists.txt, please remove one of them.

- As I told you before:
+ channelInfo[static_cast<int>(ChannelColors::navy)]="Used for SlopeWalk logging";
+ channelInfo[static_cast<int>(ChannelColors::blue)]="Used for Culling Tile mechanism logging";

^ This above is the very evidence the channels shouldn't use colors but more meaningful names. Please consider renaming the used ones with more useful names for the filenames and internal channel names. (The color used can remain, of course.)
I also wondered whether the channel description could be logged as the first thing in the corresponding file, so that when you open the file, what's inside is first described. What do you think?

- Consider renaming ChannelColors to DebugChannelColors.
- While I see it, please don't use ';' at the end of function declarations, only for classes and structs.
- Consider changing the 17 magic number with a more meaningfull const int member, such as const int NUM_DBGCHANNELS = 17;
- Consider adding m in front of class members. I.e: std::array<string, 17> channelInfo; -> std::array<string, 17> mChannelInfo;
- Consider renaming CullingManager::newBashAndSplashTiles(__int128 mode) -> CullingManager::bashAndSplashTiles(__int128 mode) since it's now the only function there.
- Consider using the Allman style, even for headers, please. http://en.wikipedia.org/wiki/Indent_style#Allman_style

Once we have all this settled out, I'll be ok for you to merge.

When merging, have an extra care not to add gui/ files to the code repo, btw, and when merging the gui files in SVN, please have the care not do delete the recent work done on
the gui files.

Could you also remove the paul424-stacktrace/ branch if you have finished with it?
What are you going to do about the image2map/ branch?

Regards,
Last edited by Bertram on 27 Sep 2014, 12:41, edited 2 times in total.
User avatar
Bertram
VT Moderator
 
Posts: 1652
Joined: 09 Nov 2012, 12:26

Re: OD - Ordered TODO list thread

Postby paul424 » 04 Jul 2014, 11:03

Could you also remove the paul424-stacktrace/ branch if you have finished with it?
What are you going to do about the image2map/ branch?

Hmm I certainly could remove the stacktrace branch , what comes to the second one
Wel it;s the old dilemma : how to eat a cake and still have it :\ .
As the results were put into development I could remove image2map and start the new one from the devel. Donno if such dance is what's profesionals do ? Or should I resync from development to the image2map ....
EDIT : done reformatting, although I couldn't find broken intendation in SlopeWalk apart wrong "{' and '}' Could you explain what you had in mind ?
EDIT: After further investigetions I learned that channel debug and debug channel is not the same in English :P , and if we mean channel for debugging purposes the second name is the correct one ...

Hmm the file you mention is laced back and forth with '\t' :
{l Code}: {l Select All Code}
tom@oberon:~/Opendungeons/opendungeons/gui> grep $'\t' OpenDungeons.looknfeel
   <PropertyDefinition initialValue="FFFFFFFF" name="NormalTextColour" redrawOnWrite="true" />
      <ImageProperty name="InitialImage" />
      <PropertyDefinition initialValue="FFFFFFFF" name="NormalTextColour" redrawOnWrite="true" />
      <PropertyDefinition initialValue="tl:FFFFFFFF tr:FFFFFFFF bl:FFFFFFFF br:FFFFFFFF" name="ImageColours" redrawOnWrite="true" />
      <PropertyDefinition initialValue="tl:FFFFFFFF tr:FFFFFFFF bl:FFFFFFFF br:FFFFFFFF" name="FrameColours" redrawOnWrite="true" />
      <PropertyDefinition initialValue="tl:FFFFFFFF tr:FFFFFFFF bl:FFFFFFFF br:FFFFFFFF" name="BackgroundColours" redrawOnWrite="true" />
               <Dim type="LeftEdge"><ImageDim dimension="Width" name="OpenDungeons/StaticLeft" /></Dim>
               <Dim type="TopEdge"><ImageDim dimension="Height" name="OpenDungeons/StaticTop" /></Dim>
               <Dim type="RightEdge">
                  <OperatorDim op="Subtract"><UnifiedDim scale="1" type="RightEdge" />
               <ImageDim dimension="Width" name="OpenDungeons/StaticRight" />
                     </OperatorDim>
                  </Dim>
               <Dim type="BottomEdge">
                  <OperatorDim op="Subtract"><UnifiedDim scale="1" type="BottomEdge" />
               <ImageDim dimension="Height" name="OpenDungeons/StaticBottom" />
                     </OperatorDim>
                  </Dim>
            </Area>
               <Dim type="LeftEdge"><AbsoluteDim value="0" /></Dim>
               <Dim type="TopEdge"><AbsoluteDim value="0" /></Dim>
               <Dim type="Width"><UnifiedDim scale="1" type="Width" /></Dim>
               <Dim type="Height"><UnifiedDim scale="1" type="Height" /></Dim>
            </Area>
      <PropertyDefinition initialValue="tl:FFFFFFFF tr:FFFFFFFF bl:FFFFFFFF br:FFFFFFFF" name="FrameColours" redrawOnWrite="true" />
      <PropertyDefinition initialValue="tl:FFFFFFFF tr:FFFFFFFF bl:FFFFFFFF br:FFFFFFFF" name="BackgroundColours" redrawOnWrite="true" />
    <PropertyDefinition redrawOnWrite="true" initialValue="tl:FFFF0000 tr:FFFF0000 bl:FFFF0000 br:FFFF0000" type="ColourRect" name="ImageColours"/>
      <ColourRectProperty name="ImageColours"/>
      <ColourRectProperty name="ImageColours"/>
           </Section>
            <Image component="Background" name="ODBackgrounds/Background1" />
         
      <ImagerySection name="background">
            <Image name="OpenDungeonsIcons/ButtonBackground" />
      <ImagerySection name="frame">
            <Image name="OpenDungeonsIcons/ButtonFrame" />
            <Section section="background" />
         </Layer>   
         <Layer priority="1">
            <Section section="normal" />
         </Layer>
         <Layer priority="2">
            <Section section="frame" />
         </Layer>            
         <Layer priority="0">
            <Section section="background" />
         </Layer>   
         <Layer priority="1">
            <Section section="normal" />
         </Layer>
         <Layer priority="2">
            <Section section="frame" />
         </Layer>            
         <Layer priority="3">
            <Section section="hover" />
         </Layer>
         <Layer priority="0">
            <Section section="background" />
         </Layer>   
         <Layer priority="1">
            <Section section="normal" />
         </Layer>
         <Layer priority="2">
            <Section section="frame" />
         </Layer>            
         <Layer priority="3">
            <Section section="pushed" />
         </Layer>
         <Layer priority="0">
            <Section section="background" />
         </Layer>   
         <Layer priority="1">
            <Section section="normal" />
         </Layer>
         <Layer priority="2">
            <Section section="frame" />
         </Layer>            
         <Layer priority="3">
            <Section section="hover" />
         </Layer>
            <Section section="background" />
         </Layer>   
         <Layer priority="1">
            <Section section="normal" />
         </Layer>
         <Layer priority="2">
            <Section section="frame" />
         </Layer>            
         <Layer priority="3">
            <Section section="disabled" />
         </Layer>
User avatar
paul424
OD Moderator
 
Posts: 660
Joined: 24 Jan 2012, 13:54

Re: OD - Ordered TODO list thread

Postby Bertram » 04 Jul 2014, 13:21

Hi Paul :)

Thanks for all the fixes.

As the results were put into development I could remove image2map and start the new one from the devel. Donno if such dance is what's profesionals do ? Or should I resync from development to the image2map ....

Usually, if you want to (re)start on a merged branch, I'd advise you to start clean (delete/recreate) since it would prevent you from having parasite merge commits.
The rest is a matter of taste, IMHO.

EDIT : done reformatting, although I couldn't find broken intendation in SlopeWalk apart wrong "{' and '}' Could you explain what you had in mind ?

I've checked the file after the latest commit, and it looks clean so far, so I guess either you fixed it or I've drunk too much. ;)

EDIT: After further investigetions I learned that channel debug and debug channel is not the same in English :P , and if we mean channel for debugging purposes the second name is the correct one ...

True, and I saw you removed the old Channelcolor.h file but didn't add the new DebugChannelColor.h file.
The next time, try to check your changes with both git diff and git status to see what new files needs to be added. This is preventing a lot of mistakes for me.

Btw, there is a change I don't understand in the latest commit:
{l Code}: {l Select All Code}
-void CullingManager::newBashAndSplashTiles(__int128 mode){
+int64_t CullingManager:: bashAndSplashTiles(int64_t mode )

The new function name is fine, but the function changed from void fun(__int128) to int64_t fun(int64_t).
Is that desired?

As for the gui files using tabs, I know their state is currently far from perfect but let's not add to the problem.

Regards,
User avatar
Bertram
VT Moderator
 
Posts: 1652
Joined: 09 Nov 2012, 12:26

Re: OD - Ordered TODO list thread

Postby paul424 » 04 Jul 2014, 19:41

Well doin't mind the singnicture of that function . It does not matter much ....
Well could you do the merge for me ? the git seems to be totally fool by the diffrence of one directory :( ....
User avatar
paul424
OD Moderator
 
Posts: 660
Joined: 24 Jan 2012, 13:54

SHOUTBOX

Postby paul424 » 05 Jul 2014, 15:50

Until we don't have a true shoutbox, which by decision of "elder and wiser" we don't need at all, please post your ad-hoc messages in here, instead of spamming with the content which does not fit any topic at all. Always wanted the forum threasd to be half documentation of development's process, not baazars' rumble and bumble .....

EDIT: merging with Git is so labourus tasks ... Best would be to find some extra person , who is good with git to do only that ....
EDIT2: Due to my mistake, I just deleted the previous post in this thread >_> , if you wish just tell if we need separate shoutbox.
User avatar
paul424
OD Moderator
 
Posts: 660
Joined: 24 Jan 2012, 13:54

Re: OD - Ordered TODO list thread

Postby Bertram » 06 Jul 2014, 21:44

Hi Paul,

Well doin't mind the singnicture of that function . It does not matter much ....

Why? :?

Well could you do the merge for me ? the git seems to be totally fool by the diffrence of one directory :( ....
EDIT: merging with Git is so labourus tasks ... Best would be to find some extra person , who is good with git to do only that ....

Well, I'll be honest, next time try to order your commit so that it less laborious, using interactive rebasing for instance if you didn't make some order before committing.
Also, since you made branches on SVN, why didn't you commit the gui files there?
The problem is now I don't really know what changes were done on the gui files. Can you hint me?

Also, IMHO it is a very big mistake to think somebody will happily merge commit for you and do only that for a spare-time project, and it is even worse if you ask somebody who doesn't really know the project as he/she could do merge mistakes much more easily. I'd rather not merge things you're not sure of, anyway.

Speaking of which, your latest commit is changing console command names without changing the console_commands.txt file accordingly if I'm not mistaken, and it seems the members of the channeDebug.h file don't start with m* such as mLogMessage for instance.

Could you check all the latest files you added for members and Allman style?

I'll do the laborious part afterward.

Thanks an regards,
User avatar
Bertram
VT Moderator
 
Posts: 1652
Joined: 09 Nov 2012, 12:26

Re: Re: OD - Ordered TODO list thread

Postby paul424 » 07 Jul 2014, 15:33

I own to say sorry for deleting by accident post from that thread :( .
I will sit down and try to fix the remaining errors. It might have happened that some new / renamed commands weren't changed in the same commit , but I pay attention to that file , ( I would regrep it again if everything matches) , what it comes to existing commands -- problem is , some does not work, because the AS is removed -- like the R.i.P. fps command.
Also I introduced the __int128_t type , which is not supported on my second 32 bit g++ , what should I do , scrap it them , or could we forget about i686 architecture ?
Donno, I have no exp in working with large teams, hmm my silly guess was , since there are specialist from everything , maybe there are specialists from git , which ain't good algorithmic coders. ( those have hundrets of branches ) .
EDIT: AFAIR, I couldn't branch off particualr directory, so we ended up add needed subdirs onto git repo . I modified only OpenDungeons.lookandfeel and OpenDungeons.png
User avatar
paul424
OD Moderator
 
Posts: 660
Joined: 24 Jan 2012, 13:54

Re: OD - Ordered TODO list thread

Postby Bertram » 08 Jul 2014, 09:03

Hi,

I own to say sorry for deleting by accident post from that thread :( .

As I said, mistakes can happen. We are human. But as charlie righteously said, maybe you wanted to move things too much in comparison of the actual need.
I'm taking the opportunity to say that the ordered todo list and windows binary thread are there to get feedback on the ongoing development, so if things are drifting a bit there, it's not that bad. It's just our creativity. :)

I will sit down and try to fix the remaining errors. It might have happened that some new / renamed commands weren't changed in the same commit , but I pay attention to that file , ( I would regrep it again if everything matches) , what it comes to existing commands -- problem is , some does not work, because the AS is removed -- like the R.i.P. fps command.

Here we've got something indeed. May I request that later, we resync the commands list with the functioning ones, and list which ones are dead, and look whether we can fix them?
Btw, moving the FPS command code to AS looks irrelevant to me, anyway. AS should be used for game scripting, and commands done in AS should be about the gameplay mostly, if possible.
What is too tied to the engine should remain in c++, IMHO.

Why? Because when you add scripting support, you always have to balance what is in the scripting language (more free, customizable but can be broken more easily and slower) and what remains in the engine (quicker, usually works better but static) and the best practice here is usually to open gameplay mechanisms such as creature spawning in our case, and let the rest in c++, the FPS command in our case, for instance. My two cents.

Also I introduced the __int128_t type , which is not supported on my second 32 bit g++ , what should I do , scrap it them , or could we forget about i686 architecture ?

No, dropping an architecture because of lazy code is of course out of the question and for obvious reasons. The real question is : Why did you introduce the 128 type in the first place. It seems to me you don't need it anyway.

Donno, I have no exp in working with large teams, hmm my silly guess was , since there are specialist from everything , maybe there are specialists from git , which ain't good algorithmic coders. ( those have hundrets of branches ) .

Here again, I think you're mistaken. We're not a "large team" and we're a spare-time team, so our possible coordinated amount of efforts are only even smaller. Thus, to avoid the struggle of continuously clean the code in term of policy and readability which is in itself a project killer, **all** devs should make their patch as clean as possible, not forget headers and documentation, and follow the policy so that this laborious part is done little by little and by everyone, and not put on a sole person, having all the bad part of it.
I'll be honest, if you can't code cleanly, one day or another, that will make people leave the project, or definitely fork it, and you'll be able to complain you're alone once again.

EDIT: AFAIR, I couldn't branch off particualr directory, so we ended up add needed subdirs onto git repo

I thought that you've fixed the SVN repo to add branching support and be able to do media branching for such needs?

I modified only OpenDungeons.lookandfeel and OpenDungeons.png

Ok, if and only if it's backward-compatible, can you commit that in SVN media? (and fix the tab instead of 4-spaces use problem if you can?)

Thanks and regards,
User avatar
Bertram
VT Moderator
 
Posts: 1652
Joined: 09 Nov 2012, 12:26

Re: Re: OD - Ordered TODO list thread

Postby hwoarangmy » 09 Jul 2014, 23:19

paul424 {l Wrote}:Also I introduced the __int128_t type , which is not supported on my second 32 bit g++ , what should I do , scrap it them , or could we forget about i686 architecture ?
You should not choose a fixed size type because it works on your g++ or not. You should choose it because you think it's value can reach over what the smaller types can handle. To say things clearly, if you need it, you should use it and try to find a way to make it work everywhere. But if you don't need it, just don't use it.

paul424 {l Wrote}:Donno, I have no exp in working with large teams, hmm my silly guess was , since there are specialist from everything , maybe there are specialists from git , which ain't good algorithmic coders. ( those have hundrets of branches ) .
The person that does the commit would have to be someone that knows well the code to be able to estimate if some code is relevant/well done or not. So it would be a big waste to have such a person to only commit other people's patches. Moreover, it usually is the coder that makes a patch that commits it because he might have to handle conflicts. And it may be hard to know what to do in such a situation. For example, if I remove a function in a patch and you release during the same time another one with a new call to this function, what shall the git specialist do ?

Releasing patches with git is not very difficult. If you take some time to go into it, you well see. The most important thing for coders is to try to have code as updated as possible because when you have to merge a patch from an old version, it is just like hell...
hwoarangmy
 
Posts: 567
Joined: 16 Apr 2014, 19:13

Who is online

Users browsing this forum: No registered users and 1 guest