Improvements and some bug fixes, will someone merge?

LazyCat · 2013-03-10T22:40:35.000Z

kenoxite:

It happened any time the y/n prompt appeared, such as when filling a container from a toilet.

You mean when you press ‘e’ to examine and it asks “Drink from your hands? (yes/no)”… it works fine here. Perhaps I fixed it in the meantime by fixing something else. Or maybe there is something different on your computer, are you using default 12x12 size for the main gameplay window? Unrelated to my changes there are issues when not using default 12x12 size, so maybe that’s just one of those.

LazyCat · 2013-03-10T22:48:31.000Z

Drakasin:

Yes, I was talking about the font’s squareness. I don’t know why, but for some reason it’s harder for me to read and look at.

Yeah it’s harder to read, but much of it is just a matter of getting used to. I think it’s worth to sacrifice some readability in order to get proper scaling of the graphics on the main gameplay window. There are also some fonts designed for 8x8 (16x16) grid, and if you find some of those they should be more readable than streched 16x8 font.

Tase · 2013-03-11T02:35:53.000Z

I made a branch and put all your stuff in here

github.com/philippeop/DDAtase

Merge remote-tracking branch 'upstream/master' into lazycat

Conflicts:
	iuse.cpp

by philippeop on 09:39PM - 15 Mar 13

changed 21 files with 704 additions and 524 deletions.

Check it out, tell me if I missed something:

github.com/philippeop/DDAtase

Intial commit of LazyCat's work

by philippeop on 02:25AM - 11 Mar 13

changed 10 files with 12716 additions and 12493 deletions.

LazyCat · 2013-03-11T07:38:46.000Z

[quote=“Tase, post:23, topic:406”]I made a branch and put all your stuff in here

github.com/philippeop/DDAtase

Merge remote-tracking branch 'upstream/master' into lazycat

Conflicts:
	iuse.cpp

by philippeop on 09:39PM - 15 Mar 13

changed 21 files with 704 additions and 524 deletions.

Check it out, tell me if I missed something:

https://github.com/tasetase/DDAtase/commit/8229a0a31a72a4b553fba0b5d91a6b1967aef5d5[/quote]

I think that’s about it. Is that merged with the most recent files from the main branch?

There is some extra stuff. In crafting.h for example someone else made changes in the last few days, I didn’t even touch that file and yet it reports a difference as if I want to revert those changes back to like it was few days ago.

I have no idea how to use it. Can I upload modifications and updates there? Need to open account? Anything else I should know about it?

Shades · 2013-03-11T08:17:20.000Z

Whats with that change to posix_time.h?

Can we also remove the //CAT: and lines of code commented out then replaced. We have history anyway so don’t need to do that.

There appear to be a number of lights in the map::draw function marked as changed when they were just reordered (was there a good reason for that?) or removed completely and I’m not sure they should have been, looks like it’s overwritten some of kevins recent changes there too.

Also the lightmap changes for vehicle parts, but I assume this is just from the original change set.

LazyCat · 2013-03-11T10:59:15.000Z

Shades:

Whats with that change to posix_time.h?

I haven’t touched posix_time.h but I did disable nanosleep() in posix_time.cpp. I think it’s desirable to have walking and driving animations be as fast as possible. And for other animations that do need some slow down so we can even see them, like bullets and explosions, then Sleep() should do just fine.

Can we also remove the //CAT: and lines of code commented out then replaced. We have history anyway so don't need to do that.

It’s up to you to merge whatever you like, whichever way you like, but if possible leave those comments in lazycat branch. Those tags help me jump around through files and keep track of what parts to update in case I need to make some modification to the whole thing.

There appear to be a number of lights in the map::draw function marked as changed when they were just reordered (was there a good reason for that?) or removed completely and I'm not sure they should have been, looks like it's overwritten some of kevins recent changes there too.
Also the lightmap changes for vehicle parts, but I assume this is just from the original change set.

As far as I figure out what is map:draw supposed to do and how it should look like I found lots of stuff there have no real purpose and make no difference one way or another, or produce bugs, so I removed quite a bit of stuff from there completely, and as much as I tested it works fine (better) like that. I’m just not sure if I tested all the possible cases and scenarios.

Ultimately it’s up to you what to merge and how, but to fix flashlight you need to look at this part:

/*
   // Don't display area as shadowy if it's outside and illuminated by natural light
   else if (dist <= g->u.sight_range(g->natural_light_level())) 
   {
       lowlight_sight_range = std::max(g_light_level, natural_sight_range);
   } 
*/

Just comment that bit out, or fix it otherwise, to get proper dark shade ring around the flashlight. Leave the rest if you like, I just don’t see how it matters and what is it supposed to do. I plan to re-write and improve that whole thing anyway, especially in relation to light/shadows, flashlights, candles, night vision and such, so I deleted it to easier look at it.

Shades · 2013-03-11T14:24:49.000Z

LazyCat:

I haven’t touched posix_time.h but I did disable nanosleep() in posix_time.cpp. I think it’s desirable to have walking and driving animations be as fast as possible. And for other animations that do need some slow down so we can even see them, like bullets and explosions, then Sleep() should do just fine.

The commit had a return 0; at the start of the posix_time function which was why I queried it.

LazyCat:

As far as I figure out what is map:draw supposed to do and how it should look like I found lots of stuff there have no real purpose and make no difference one way or another, or produce bugs, so I removed quite a bit of stuff from there completely, and as much as I tested it works fine (better) like that. I’m just not sure if I tested all the possible cases and scenarios.

That is possible there are certainly issues in that set of code, originally it was all meant to do stuff but it’s likely removing made no difference. I haven’t testing it myself

Tase · 2013-03-11T14:45:37.000Z

The crafting.h thing is my mistake, I’ll revert it once I get home.

As for //CAT: I can just remove it before making a pull request.

The code is up to date with the latest, at least whatever was latest last night.

I can add you as a contributor to the branch but you have to make an account and get git to do anything.

kevin.granade · 2013-03-11T18:40:54.000Z

FYI, I tried to review your changes, but you shuffled things around so much that it’s too much trouble to review. If you want this considered for inclusion, please either post clean patches with just the functional changes, or make a github account and make some clean commits.

By clean I mean without extraneous whitespace, line ordering, or bracketing style changes. Those make the changes too difficult to review. I can’t tell what you did in map::draw(), which is the part I’m most interested in.

LazyCat · 2013-03-11T18:53:42.000Z

Shades:

The commit had a return 0; at the start of the posix_time function which was why I queried it.

Yes, that’s what I meant by “disabled nanosleep()”. Try it with and without and and tell me what do you think is better and why would there be any sleep at all.

LazyCat · 2013-03-11T19:00:19.000Z

[quote=“Tase, post:28, topic:406”]The crafting.h thing is my mistake, I’ll revert it once I get home.

As for //CAT: I can just remove it before making a pull request.

The code is up to date with the latest, at least whatever was latest last night.

I can add you as a contributor to the branch but you have to make an account and get git to do anything.[/quote]

It seems to me you then able to merge any of those changes, all I need is some way to upload updates to lazycat branch and then you can merge with the master whatever you like whenever you like. As far as I am concern I can upload files to Mediafire or directly to git, whichever suits you.

LazyCat · 2013-03-11T20:01:03.000Z

Kevin Granade:

FYI, I tried to review your changes, but you shuffled things around so much that it’s too much trouble to review. If you want this considered for inclusion, please either post clean patches with just the functional changes, or make a github account and make some clean commits.

I think my changes are extremely simple and clean. Can you be more specific, what is confusing you, exactly?

By clean I mean without extraneous whitespace, line ordering, or bracketing style changes. Those make the changes too difficult to review. I can't tell what you did in map::draw(), which is the part I'm most interested in.

Ay, caramba! White-spaces and proper bracketing is what makes it easier to read. Anyway, in map:draw I commented out these lines:

/* // Don't display area as shadowy if it's outside and illuminated by natural light else if (dist <= g->u.sight_range(g->natural_light_level())) { lowlight_sight_range = std::max(g_light_level, natural_sight_range); } */

As I said that’s all it matters to fix the bug. Everything else that is there or other things that are missing is just bunch of stuff for my personal musings. All that other stuff which I did not mention when I explained these changes were not meant for merge, they just happen to be there as well. Eventually, in the meantime, it became this (rounded lights):

Flashlight, outdoors and indoors.

Three flashlights and a candle on the ground, holding a torch.

Reservoir · 2013-03-11T21:21:42.000Z

Did you fix the craft thing? I cant board up windows or move furniture

Tase · 2013-03-11T21:46:17.000Z

[quote=“LazyCat, post:31, topic:406”][quote=“Tase, post:28, topic:406”]The crafting.h thing is my mistake, I’ll revert it once I get home.

As for //CAT: I can just remove it before making a pull request.

The code is up to date with the latest, at least whatever was latest last night.

I can add you as a contributor to the branch but you have to make an account and get git to do anything.[/quote]

It seems to me you then able to merge any of those changes, all I need is some way to upload updates to lazycat branch and then you can merge with the master whatever you like whenever you like. As far as I am concern I can upload files to Mediafire or directly to git, whichever suits you.[/quote]

It’s not as simple as uploading files, you need to connect to git with a client to upload your modifications.

Anyway, for now you can upload (mediafire) a clean map.cpp with just your changes (no reordering variables, no changing indentation, no new brackets, just different numbers and characters) so Kevin can check out what you did in map.cpp.

LazyCat · 2013-03-12T03:39:29.000Z

Reservoir:

Did you fix the craft thing? I cant board up windows or move furniture

This is not release build, only to test the change mentioned in the opening post. Yes, moving furniture has been fixed since I made the compile, it works in this build Tase merged yesterday. I have nothing to do with how crafting works or doesn’t work, my changes could only impact whether display gets refreshed properly.

EDIT: Ooops, funny enough moving furniture actually still doesn’t work in the master build. Have no idea why does it work in my branch then, Tase mergining somehow fixed it.

EDIT2: Nope, it sometimes works and sometimes it doesn’t, for some strange reason. Try to restart the game or start a new one and maybe it will work.

LazyCat · 2013-03-12T04:38:55.000Z

[quote=“Tase, post:34, topic:406”]It’s not as simple as uploading files, you need to connect to git with a client to upload your modifications.

Anyway, for now you can upload (mediafire) a clean map.cpp with just your changes (no reordering variables, no changing indentation, no new brackets, just different numbers and characters) so Kevin can check out what you did in map.cpp.[/quote]

Just comment out these lines:

/*
   // Don't display area as shadowy if it's outside and illuminated by natural light
   else if (dist <= g->u.sight_range(g_light_level)) {
    low_sight_range = std::max(g_light_level, natural_sight_range);
   }
*/

He needs to check his own code, it’s not me who wrote that.

I write code the way I write, and it’s much better than how you guys write it. I will not lower my standard to suit that awful formatting. I don’t see how such trivial things as indentation and bracketing matter anyway, you can make it ugly yourself, plus you can always make those changes from scratch if you think that would be easier. I can’t be any more helpful than to give you the source code. Take it or leave it.

Shades · 2013-03-12T08:24:39.000Z

LazyCat:

I write code the way I write, and it’s much better than how you guys write it.

Yes the style we have inherited is poor, and it’s slowly being shifted but we’d rather do it function at a time as people make major changes. Besides which it’s just common courtesy to match your changes to whomever’s code your modifying, it’s not like it’s hard.

LazyCat · 2013-03-12T09:10:14.000Z

Shades:

[quote=“LazyCat, post:36, topic:406”]I write code the way I write, and it’s much better than how you guys write it.

Yes the style we have inherited is poor, and it’s slowly being shifted but we’d rather do it function at a time as people make major changes. Besides which it’s just common courtesy to match your changes to whomever’s code your modifying, it’s not like it’s hard.[/quote]

When it is properly formatted with tabs you can still use editor settings to display it the way you want it. But when you use spaces then you can not, at least my editor can’t. If you prefer to waste time typing multiple spaces instead of one tab stroke that’s fine, but don’t ask me to do it, it’s unproductive and impractical, it’s wrong way to do it. Why would I make things worse and harder for myself? That would be insane. It’s enough trouble I have to unscramble it first in order read it properly, to see where one thing ends and another one begins, due to that terrible bracketing style.

I will not be merging this anyway, and who ever does can format it however they like, rename variables and do other such modification. I need my copy of the code to be functional and practical for me. But what are we even talking about here, I just wrote this block of code:

	float cat_vel= veh->velocity*1.5;
	if(cat_vel > 10000) cat_vel= 10000;
	float cat_sin = sin(veh->turn_dir*M_PI/180)* cat_vel;
	float cat_cos = cos(veh->turn_dir*M_PI/180)* cat_vel;

	u.view_offset_y= (int)cat_sin/1000;
	u.view_offset_x= (int)cat_cos/1000;

The rest are one line changes and 90% of that are just commenting out existing stuff. So what are we even talking about here? You want me to write above code by using 24 space characters instead of 6 tabs, and minus one enter, like this:

    float cat_vel= veh->velocity*1.5;
    if(cat_vel > 10000) cat_vel= 10000;
    float cat_sin = sin(veh->turn_dir*M_PI/180)* cat_vel;
    float cat_cos = cos(veh->turn_dir*M_PI/180)* cat_vel;
    u.view_offset_y= (int)cat_sin/1000;
    u.view_offset_x= (int)cat_cos/1000;

Well, there it is.

Shades · 2013-03-12T12:32:33.000Z

If your using tabs, which is also my preferred method, you can convert to the single space system by just piping your changes through [tt]tr ‘\t’ ’ '[/tt] before sharing it for a merge.

LazyCat · 2013-03-12T18:24:42.000Z

Shades:

If your using tabs, which is also my preferred method, you can convert to the single space system by just piping your changes through [tt]tr ‘\t’ ’ '[/tt] before sharing it for a merge.

Perhaps if it was me making changes to the master branch, but it’s not. Beside there is nothing to reformat, it’s all at least as good as the rest of the code where you already have anything from one to four spaces indents, and tabs, and both styles of bracketing. This is non-issue and not worth talking about. Whoever is reviewing, copy pasting or re-creating these changes can in the same time format it and rename variables anyway they like. There are bugs, and there are bug fixes. Apply or use those bug fixes any way you want, just fix the bugs already, that’s all it matters.