| Time |
Nick |
Message |
| 00:04 |
est31 |
OldCoder, commented |
| 00:07 |
OldCoder |
est31, paramat doesn't want the maximum range capped. If the actual setting is capped, will this contradict his point? |
| 00:08 |
OldCoder |
If it is capped dynamically as you suggest, will he still be able to do as he wishes? |
| 00:08 |
paramat |
no est's suggestion is good |
| 00:08 |
OldCoder |
So, a dynamic cap? |
| 00:08 |
paramat |
there's no need to see beyond the world edge |
| 00:09 |
* OldCoder |
is curious about what is beyond, but very well |
| 00:09 |
OldCoder |
Cap needs to be dynamic so that screenshots will work as you wish, paramat ? |
| 00:09 |
OldCoder |
Will look into this |
| 00:10 |
paramat |
yes |
| 00:10 |
est31 |
capping to world edge would be even too restrictive |
| 00:10 |
OldCoder |
est31, go on |
| 00:10 |
est31 |
capping to coord edge is better |
| 00:10 |
OldCoder |
Distinction? |
| 00:11 |
est31 |
world edge is at 31000 |
| 00:11 |
est31 |
there is where mapgen stops |
| 00:11 |
OldCoder |
And coord edge is 32767 ? |
| 00:11 |
est31 |
coord edge is where s16 flips over |
| 00:11 |
est31 |
yup |
| 00:11 |
OldCoder |
Ninja |
| 00:11 |
est31 |
:) |
| 00:11 |
OldCoder |
Ttys |
| 00:11 |
paramat |
yes agreed, coord edge |
| 00:11 |
* OldCoder |
will rest and look at this |
| 00:11 |
OldCoder |
o/ |
| 00:11 |
est31 |
that way you still see something when standing at world edge and looking back |
| 00:11 |
OldCoder |
Indeed |
| 00:12 |
OldCoder |
"I don't know where I'm going but can see where I've been" |
| 00:31 |
|
Taoki joined #minetest-dev |
| 00:36 |
|
H-H-H joined #minetest-dev |
| 00:48 |
|
paramat left #minetest-dev |
| 00:52 |
|
STHGOM joined #minetest-dev |
| 00:54 |
|
H-H-H joined #minetest-dev |
| 01:37 |
|
RealBadAngel joined #minetest-dev |
| 01:49 |
OldCoder |
est31, patch has been updated to reflect both your input and paramat's |
| 01:50 |
OldCoder |
https://github.com/minetest/minetest/pull/3659 |
| 01:50 |
OldCoder |
^ Ready for glory |
| 01:50 |
OldCoder |
Press Reload button or Refresh link on page to see latest version |
| 01:50 |
OldCoder |
|
| 01:51 |
* OldCoder |
has spent a day on this and looks forward, eventually, to his first official patch |
| 01:52 |
OldCoder |
Hm, wait, the update didn't flush |
| 01:52 |
* OldCoder |
goes to fix it |
| 01:53 |
OldCoder |
No, it's correct, press Reload |
| 01:59 |
|
kaadmy joined #minetest-dev |
| 02:01 |
est31 |
OldCoder, commented |
| 02:02 |
OldCoder |
est31, thanks |
| 02:02 |
OldCoder |
Commenting back |
| 02:03 |
OldCoder |
This is fun, in a way. II suppose I'll end up with a parallel program as many developers, including you, have. |
| 02:03 |
OldCoder |
Oops |
| 02:03 |
OldCoder |
This is fun, in a way. I'm convinced I know best, some will agree and some will not. I suppose I'll end up with a parallel program as many developers, including you, have. |
| 02:03 |
OldCoder |
* |
| 02:03 |
OldCoder |
Comments coming online |
| 02:10 |
OldCoder |
est31, that last point is sensible |
| 02:10 |
OldCoder |
I'll probably fix that, but growing physically tired today |
| 02:10 |
* OldCoder |
is referring to the per-frame issue |
| 02:10 |
est31 |
ah yeah |
| 02:10 |
est31 |
it will make the code more complex, that's bad |
| 02:10 |
est31 |
but any fix is probably better than no one :) |
| 02:10 |
OldCoder |
Incorrect, it's good |
| 02:11 |
OldCoder |
Will explain online as paramat has raised this issue there |
| 02:11 |
OldCoder |
Will fix the runtime load issue separately another day, I think |
| 02:12 |
est31 |
take your time |
| 02:14 |
|
paramat joined #minetest-dev |
| 02:15 |
paramat |
sorry OldCoder i don't think it's worth fixing at all |
| 02:16 |
OldCoder |
paramat, I am savaging your views in the PR thread |
| 02:16 |
OldCoder |
As you have commented there |
| 02:16 |
OldCoder |
I trust you will not mind |
| 02:16 |
* OldCoder |
turns to GitHub and edits |
| 02:17 |
paramat |
no problem |
| 02:22 |
OldCoder |
paramat, a confusing point... |
| 02:22 |
OldCoder |
I don't think it's even worth fixing, it would be preferable to keep the ability of a 2000 node view range elsewhere in the world. |
| 02:22 |
OldCoder |
^ quote |
| 02:22 |
OldCoder |
The patch does exactly that as we discussed |
| 02:22 |
OldCoder |
It only limits view range at the edges of the universe |
| 02:23 |
OldCoder |
Am I missing something or was this not clear? |
| 02:23 |
paramat |
yes, i'm referring to my suggested alternative fix |
| 02:24 |
est31 |
I hope we agree there are two alternatives here: |
| 02:24 |
est31 |
1. limit the viewing range to a fixed amount |
| 02:24 |
est31 |
2. limit the viewing range to the distance to the world edge |
| 02:25 |
est31 |
2 is more smarter and is almost as lightweight as 1. |
| 02:25 |
OldCoder |
My online response will go into detail regarding one issue because I feel it's an important philosophical difference |
| 02:25 |
kahrl |
well there's a third one |
| 02:25 |
paramat |
do nothing |
| 02:25 |
OldCoder |
But I'll work further on the patch and wish to hear kahrl's point |
| 02:25 |
kahrl |
yes, and a fourth one |
| 02:25 |
OldCoder |
paramat, you have made your view clear |
| 02:25 |
kahrl |
work with v3s32's and rangelim them to s16 range at the end |
| 02:25 |
kahrl |
that would give the most "correct" output, but I'm not sure how expensive it is |
| 02:26 |
OldCoder |
You're in error and this will be explained shortly. However, your time is also much appreciated. |
| 02:26 |
OldCoder |
Not kahrl, paramat |
| 02:26 |
OldCoder |
This has been more fun than these things usually are |
| 02:28 |
kahrl |
4. is more correct than 2., because let's say the viewing range is 3000 and you're at the world border, if you look away from the world border you see 3000 nodes far with option 4., but less with 2. |
| 02:30 |
OldCoder |
est31, paramat, reading the exchange, I'd like to be clear, this is all with a smile. I've genuinely enjoyed the process and appreciated your help. Both of you. |
| 02:30 |
paramat |
ok |
| 02:30 |
OldCoder |
kahrl, I'm growing physically tired but will review your remarks at a later point. |
| 02:32 |
est31 |
OldCoder, paramat and me often play this game "est wants this, paramat doesnt want this", its not the first time we have discussions of this kind. |
| 02:32 |
OldCoder |
Ah thanks |
| 02:32 |
* OldCoder |
turns back to editing |
| 02:33 |
paramat |
heh yeah |
| 02:34 |
paramat |
my opinion is shifting anyway |
| 02:34 |
est31 |
its not really personal, but idk, our views arent aligned or something. still paramat is a nice guy |
| 02:35 |
paramat |
est is ok too |
| 02:39 |
kahrl |
so, while we're looking at the performance of this code |
| 02:39 |
kahrl |
what is the purpose of m_camera_mutex? |
| 02:39 |
kahrl |
all places that lock it appear to be called from the main thread |
| 02:40 |
kahrl |
ClientMap::m_camera_mutex, to be clear |
| 02:43 |
kahrl |
I grepped the codebase for lines that lock it (thankfully grep only found lines in clientmap.h and clientmap.cpp) and added logging next to each one |
| 02:43 |
kahrl |
the log only shows [Main] as the thread |
| 02:43 |
OldCoder |
est31, I'll drop the redundant code if you'll merge the P.R. I've explained the reasons for retaining it but it isn't a significant issue. |
| 02:45 |
OldCoder |
I'll also look at a more efficient version at a later date, though it sounds as though kahrl is onto something |
| 02:45 |
OldCoder |
I actually considered that approach this morning, but didn't know enough to go that route |
| 02:46 |
OldCoder |
Or you can merge the P.R. and delete those lines |
| 02:46 |
OldCoder |
est31, ^ |
| 02:47 |
paramat |
i'm beginning to agree this needs attention, but i think we can come up with something better, i don't think this is mergeable |
| 02:47 |
kahrl |
gaining performance by getting rid of an unnecessary mutex would compensate a somewhat more expensive node range calculation :) |
| 02:47 |
est31 |
yup |
| 02:47 |
est31 |
I'll try to dig up the commit that added the mutex |
| 02:49 |
OldCoder |
paramat, I'm fine with tossing the P.R. and am glad if it leads to discussion of a superior approach |
| 02:50 |
paramat |
ok cool |
| 02:50 |
* OldCoder |
has not eaten and will now go on break. He will be here Saturday night later and all day Sunday. |
| 02:51 |
OldCoder |
o/ |
| 02:51 |
OldCoder |
(This assumes that the issue is fixed one way or the other) |
| 02:51 |
OldCoder |
|
| 02:51 |
kahrl |
est31, it's there in 4e249fb |
| 02:51 |
kahrl |
"Initial files" |
| 02:54 |
kahrl |
wow, I completely forgot about the initial attempt to add lua scripting (way before 0.4 was even dreamt of) |
| 02:55 |
kahrl |
(got reminded by commit 9778347c which touches m_camera_mutex) |
| 02:57 |
paramat |
not sure if this is the case, but the camera seems to have a viewing limit of 2000 nodes? if so, for a start we could limit view range min/max and m_control.wanted_range to 2000 nodes, this would fix this occuring in all places more than 300 nodes from world edge |
| 02:57 |
paramat |
this would be lightweight |
| 02:58 |
paramat |
another possibility, reduce world size by 300 nodes =) |
| 02:58 |
paramat |
or to 30000 (might not be popular though) |
| 03:01 |
* OldCoder |
is headed out to lunch but hopes not to sacrifice outer 1000 nodes just for this. |
| 03:02 |
paramat |
yeah forget that idea |
| 03:02 |
paramat |
players already say it's too small |
| 03:02 |
OldCoder |
Tell them to build up :P |
| 03:04 |
paramat |
so since this occured due to m_control.wanted_range becoming 9600+ first we should avoid such crazy values being possible. limiting this and view range min/max can be done where those are set or adjusted, lightweight because not per-frame |
| 03:06 |
est31 |
well kahrl m_camera_mutex is an object that's predating the git history of minetest: its contained in the initial commit 4e249fb |
| 03:08 |
paramat |
!tell RealBadAngel does the camera have a viewing limit of 2000 nodes? |
| 03:08 |
ShadowBot |
paramat: O.K. |
| 03:12 |
kahrl |
est31: yeah |
| 03:12 |
kahrl |
it's not there in NMPR, so it must have been added between then and the first git commit |
| 03:13 |
|
RealBadAngel joined #minetest-dev |
| 03:15 |
RealBadAngel |
paramat, near plane of camera is set to 1.0 i think by default |
| 03:15 |
RealBadAngel |
mt never sets or changes near plane |
| 03:15 |
RealBadAngel |
also minimum is always 20000 |
| 03:15 |
RealBadAngel |
(for far plane) |
| 03:16 |
RealBadAngel |
so it can 1 - 20000 or 1 - 20000+ |
| 03:16 |
est31 |
well kahrl I can see m_camera_mutex be unlocked / locked at several places: |
| 03:17 |
est31 |
1. in the header src/clientmap.h. The only place where updateCamera( is called is in game.cpp and that code is only called in main game loop |
| 03:18 |
est31 |
2. ClientMap::renderPostFx is only called by draw_scene in drawscene.cpp |
| 03:18 |
est31 |
and the method has video::IVideoDriver *driver as its argument |
| 03:18 |
est31 |
knowing that irrlicht is single threaded only, it must be used in the main thread already |
| 03:19 |
paramat |
hm i know clouds cutoff at 2000 nodes |
| 03:19 |
est31 |
3. ClientMap::updateDrawList and ClientMap::renderMap both take the driver as arguments as well. same argument applies |
| 03:19 |
kahrl |
ah, true I didn't even think of that to make it easier to analyze |
| 03:19 |
est31 |
so +1 to your proposal |
| 03:20 |
paramat |
i suspect MT sets far plane to 20000 units = 2000 nodes |
| 03:20 |
RealBadAngel |
paramat, see what i wrote |
| 03:20 |
RealBadAngel |
20000 is hardcoded limit |
| 03:21 |
RealBadAngel |
whatever distance you set, far plane of camera will be no lower than 20k |
| 03:21 |
paramat |
yes |
| 03:21 |
est31 |
it'd be cool to see player's reactions when we limit world to +-11760 in all directions |
| 03:22 |
RealBadAngel |
also, propably even when you set vRange to 245 it is still 20k |
| 03:22 |
RealBadAngel |
dunno why |
| 03:22 |
paramat |
i'm just wondering what MT sets the far plane to? |
| 03:22 |
RealBadAngel |
damn |
| 03:22 |
RealBadAngel |
20k |
| 03:22 |
RealBadAngel |
always |
| 03:22 |
paramat |
well clouds are always visible out to 2000 nodes whatever view range is |
| 03:23 |
RealBadAngel |
even when you set it to 245, far plane is 20k |
| 03:23 |
RealBadAngel |
that may be a bug |
| 03:23 |
paramat |
RBA, you weote '20000+' and 'no lower than 20000', that means it could be higher? |
| 03:23 |
paramat |
(wrote) |
| 03:24 |
RealBadAngel |
lookin at code its broken |
| 03:24 |
RealBadAngel |
20k means that far plane is way too far |
| 03:24 |
RealBadAngel |
and has nothing to do with nodes visible |
| 03:25 |
RealBadAngel |
its just set absurdly far to make everything visible |
| 03:25 |
paramat |
but clouds need to be visible out to 20000 whatever view range is set to? |
| 03:25 |
paramat |
and indeed they are |
| 03:25 |
RealBadAngel |
clouds yeah |
| 03:25 |
RealBadAngel |
but world? |
| 03:27 |
RealBadAngel |
its set to see 125 maplocks in straight line |
| 03:28 |
RealBadAngel |
20000 / BS / 16 = 125 |
| 03:29 |
RealBadAngel |
it means that setting lower vRange wont speed up rendering |
| 03:29 |
RealBadAngel |
because no matter what everything will be always rendered |
| 03:30 |
RealBadAngel |
with 20k far clipping is effectively disabled |
| 03:30 |
paramat |
but setting lower view range does speed up rendering in-game |
| 03:31 |
RealBadAngel |
blocks are manually occluded |
| 03:32 |
RealBadAngel |
in clientmap.cpp |
| 03:32 |
kahrl |
est31: #3661 |
| 03:32 |
ShadowBot |
https://github.com/minetest/minetest/issues/3661 -- Remove ClientMap::m_camera_mutex by kahrl |
| 03:32 |
RealBadAngel |
thx to that code we do have missing blocks in the screen corners sometimes |
| 03:33 |
RealBadAngel |
btw, #3654 |
| 03:33 |
ShadowBot |
https://github.com/minetest/minetest/issues/3654 -- Use tangent space meshes only when shaders are enabled by RealBadAngel |
| 03:33 |
RealBadAngel |
can somebody merge it? |
| 03:34 |
paramat |
i need to test that |
| 03:34 |
paramat |
i'll do that now |
| 03:34 |
RealBadAngel |
there are 2 dev votes already |
| 03:34 |
est31 |
but paramat wants to test it |
| 03:34 |
RealBadAngel |
ok |
| 03:35 |
paramat |
need to check it restores FPS |
| 03:35 |
est31 |
and if he finds something that needs improvement, it improves the whole situation |
| 03:35 |
est31 |
so no need for haste |
| 03:35 |
est31 |
paramat, if the test is successful, I assume you plan to merge it? |
| 03:35 |
RealBadAngel |
est31, kinda, im blocked with it |
| 03:35 |
paramat |
sorry i was going to do that earlier but got sidetracked |
| 03:36 |
est31 |
because I am planning to getting sidetracked myself |
| 03:36 |
paramat |
yeah! if it works i want it in |
| 03:36 |
est31 |
sort of... getting to bed :) |
| 03:36 |
paramat |
yeah i'll merge it if tests well |
| 03:37 |
RealBadAngel |
paramat, please do, this PR blocks all my other ones |
| 03:38 |
|
est31 joined #minetest-dev |
| 03:38 |
est31 |
and somebody if they have time should have a look at https://github.com/minetest/minetest/pull/3638 |
| 03:38 |
est31 |
its a good pr IMO and fixes a long standing bug in minetest |
| 03:38 |
* est31 |
now really heading to bed |
| 03:48 |
RealBadAngel |
paramat, ive solved problem with selection boxes, they can remain old way |
| 03:49 |
paramat |
nice |
| 03:49 |
RealBadAngel |
https://github.com/RealBadAngel/minetestHD/commit/b98cb03d7bacfcc179e7d19b629fa9eead6259cc |
| 03:49 |
RealBadAngel |
so theyre no longer an obstacle for deferred rendering |
| 03:51 |
RealBadAngel |
now i have to take care of player name tags |
| 03:52 |
RealBadAngel |
and the entities ones |
| 03:52 |
RealBadAngel |
and the sky ;) |
| 04:08 |
paramat |
ok FPS up from 42 to 52, 42 is 81% of 52 so a 19% drop, close enough to 21% so PR seems good, will merge |
| 04:10 |
RealBadAngel |
good |
| 04:10 |
paramat |
now merging |
| 04:17 |
paramat |
merged |
| 04:21 |
paramat |
issue closed too |
| 04:21 |
paramat |
thanks for that |
| 04:22 |
RealBadAngel |
np |
| 04:28 |
RealBadAngel |
i will update now 3650 |
| 04:28 |
OldCoder |
paramat, does this address the other issue or is it still open? |
| 04:37 |
paramat |
which other issue? |
| 04:38 |
OldCoder |
The one we spent an hour on (paramat) |
| 04:39 |
paramat |
ah, no it's unrelated |
| 04:40 |
OldCoder |
ah |
| 04:40 |
* OldCoder |
trusts est31 paramat others will settle the other issue. Or merge OldCoder patch as a fallback. But OldCoder will nap now. |
| 04:43 |
paramat |
now that est has reviewed 3610 it may be mergeable once updated |
| 04:54 |
RealBadAngel |
im fixing now 3650 code with nrzkt comments |
| 04:54 |
RealBadAngel |
i will do 3610 as next |
| 04:57 |
|
kaeza joined #minetest-dev |
| 05:31 |
RealBadAngel |
ok 3650 now become #3662 |
| 05:31 |
ShadowBot |
https://github.com/minetest/minetest/issues/3662 -- Cleanup selection mesh code, add shaders for halo and selection boxes by RealBadAngel |
| 05:32 |
RealBadAngel |
paramat, ^^ |
| 05:36 |
paramat |
ok |
| 05:51 |
RealBadAngel |
3610 and 3662 are confilcting as usual |
| 05:51 |
RealBadAngel |
please merge 3662 before i can make 3610 mergeable |
| 05:55 |
paramat |
ok |
| 05:58 |
paramat |
hopefully it can get some approves later on sunday |
| 05:58 |
RealBadAngel |
code was reviewed already |
| 06:03 |
paramat |
needs +1 from the reviewer nerzhul plus another +1, unfortunately you'll have to wait, much as i'd like to get things moving |
| 06:06 |
paramat |
wow big commit |
| 06:11 |
|
paramat left #minetest-dev |
| 06:34 |
|
jordan4ibanez joined #minetest-dev |
| 06:36 |
RealBadAngel |
ok, ive updated #3610 with est31 comments |
| 06:36 |
ShadowBot |
https://github.com/minetest/minetest/issues/3610 -- Use meshes to display inventory items by RealBadAngel |
| 07:34 |
|
Hunterz joined #minetest-dev |
| 08:04 |
|
Krock joined #minetest-dev |
| 08:31 |
|
nrzkt joined #minetest-dev |
| 08:47 |
|
Player_2 joined #minetest-dev |
| 08:51 |
|
Gael-de-Sailly joined #minetest-dev |
| 08:54 |
|
ud1_ joined #minetest-dev |
| 09:33 |
|
Player-2 joined #minetest-dev |
| 09:44 |
|
CraigyDavi joined #minetest-dev |
| 10:11 |
|
Obani joined #minetest-dev |
| 10:11 |
|
leat joined #minetest-dev |
| 10:24 |
|
blaze joined #minetest-dev |
| 11:02 |
|
Calinou joined #minetest-dev |
| 11:11 |
|
ud1 joined #minetest-dev |
| 11:22 |
|
jin_xi joined #minetest-dev |
| 12:51 |
|
bonnob joined #minetest-dev |
| 13:06 |
|
turtleman joined #minetest-dev |
| 13:10 |
|
leat joined #minetest-dev |
| 13:16 |
|
Fixer joined #minetest-dev |
| 13:27 |
|
kaeza joined #minetest-dev |
| 13:33 |
|
red-001 joined #minetest-dev |
| 13:57 |
|
est31 joined #minetest-dev |
| 13:57 |
|
blaze joined #minetest-dev |
| 13:59 |
red-001 |
nrzkt I updated #3657 |
| 13:59 |
ShadowBot |
https://github.com/minetest/minetest/issues/3657 -- Allow players to drop unknown items and add a setting to remove them on drop. by red-001 |
| 13:59 |
est31 |
sfan5, is the +1 you did in RBA's PR a "real" +1? |
| 13:59 |
red-001 |
could you re-review it? |
| 14:01 |
|
DFeniks joined #minetest-dev |
| 14:10 |
Fixer |
I really hope for PR #3612, i playing with it for a long time, works fine and gives nicer view |
| 14:10 |
ShadowBot |
https://github.com/minetest/minetest/issues/3612 -- Filmic HDR tone mapping by RealBadAngel |
| 14:10 |
Fixer |
I'm* |
| 14:16 |
sfan5 |
est31: what's a "real" +1? |
| 14:16 |
Fixer |
code review? |
| 14:16 |
est31 |
sfan5, well were you just supporting the idea, or were you agreeing to the actual code being merged into master |
| 14:17 |
est31 |
if its the second, I'll merge it now |
| 14:17 |
* sofar |
peeks in |
| 14:17 |
est31 |
(after reviewing it for a last time) |
| 14:17 |
sfan5 |
est31: idea approval |
| 14:17 |
sfan5 |
but i can look at the code now too if you want |
| 14:18 |
est31 |
would be cool if you did |
| 14:18 |
est31 |
137 open pull requests and counting |
| 14:19 |
red-001 |
minetest game has more PRs then issues |
| 14:19 |
sfan5 |
est31: looks good |
| 14:49 |
|
kaadmy joined #minetest-dev |
| 14:59 |
|
Obani joined #minetest-dev |
| 15:16 |
|
rubenwardy joined #minetest-dev |
| 15:25 |
Fixer |
!tell paramat happens sometimes https://github.com/minetest/minetest_game/issues/828 |
| 15:25 |
ShadowBot |
Fixer: O.K. |
| 15:41 |
|
linkedinyou joined #minetest-dev |
| 16:15 |
|
Alduin_ joined #minetest-dev |
| 16:19 |
|
casimir joined #minetest-dev |
| 16:24 |
|
rubenwardy_ joined #minetest-dev |
| 16:24 |
|
diemartin joined #minetest-dev |
| 16:34 |
|
hmmmm joined #minetest-dev |
| 16:51 |
|
Gael-de-Sailly joined #minetest-dev |
| 17:00 |
|
est31 joined #minetest-dev |
| 17:03 |
|
STHGOM joined #minetest-dev |
| 17:10 |
RealBadAngel |
so, whats about #3662, #3612 and #3610 ? |
| 17:11 |
ShadowBot |
https://github.com/minetest/minetest/issues/3662 -- Cleanup selection mesh code, add shaders for halo and selection boxes by RealBadAngel |
| 17:11 |
ShadowBot |
https://github.com/minetest/minetest/issues/3612 -- Filmic HDR tone mapping by RealBadAngel |
| 17:11 |
ShadowBot |
https://github.com/minetest/minetest/issues/3610 -- Use meshes to display inventory items by RealBadAngel |
| 17:11 |
est31 |
RealBadAngel, seen my comment in the last PR? |
| 17:11 |
est31 |
why is it not needed to set delta = 0? |
| 17:11 |
RealBadAngel |
because its set before? |
| 17:12 |
est31 |
mhh that makes sense |
| 17:12 |
est31 |
its a bit confusing what's static here and what isnt |
| 17:13 |
RealBadAngel |
this is not a class, so some variables have to be made static |
| 17:14 |
RealBadAngel |
otherwise i would need them to be global |
| 17:14 |
est31 |
why so? |
| 17:14 |
est31 |
i've thought they only serve as speedup? |
| 17:14 |
RealBadAngel |
draw function can be called from formspec or hud |
| 17:15 |
RealBadAngel |
it doesnt belong in fact to any of them, but have to remember whats selected, dragged or hovered |
| 17:16 |
RealBadAngel |
and keep the timers for those 3 |
| 17:16 |
RealBadAngel |
so its not about speed |
| 17:18 |
est31 |
and why does it require three fields? |
| 17:18 |
est31 |
why is not one enough? |
| 17:18 |
RealBadAngel |
three at the time can spin... |
| 17:18 |
est31 |
you cant hover if you drag can you |
| 17:18 |
RealBadAngel |
one in the hotbar |
| 17:18 |
RealBadAngel |
the one you have dragged |
| 17:19 |
RealBadAngel |
and you can hover next one when dragging |
| 18:17 |
|
kaeza joined #minetest-dev |
| 18:30 |
nrzkt |
RealBadAngel, i added comments on #3662 |
| 18:30 |
ShadowBot |
https://github.com/minetest/minetest/issues/3662 -- Cleanup selection mesh code, add shaders for halo and selection boxes by RealBadAngel |
| 18:33 |
RealBadAngel |
nrzkt, about frist one, scolor, that wont even compile |
| 18:33 |
RealBadAngel |
you already pointed that one before |
| 18:34 |
nrzkt |
why ? there is a local modification ? |
| 18:34 |
RealBadAngel |
idk, its not my code |
| 18:34 |
RealBadAngel |
youre pointing mostly old code around |
| 18:34 |
nrzkt |
but you are using it, adding a little maintenance could be good :) |
| 18:34 |
RealBadAngel |
ive added something to that function call |
| 18:35 |
RealBadAngel |
i have no slightest idea why it wont work |
| 18:36 |
RealBadAngel |
also why everybody are so keen on ternary operators? |
| 18:37 |
RealBadAngel |
static const on txc also wont work |
| 18:37 |
RealBadAngel |
iterators will refuse it |
| 18:37 |
hmmmm |
because having 5 lines to change one slightly different parameter in otherwise identical code is repetitive |
| 18:37 |
hmmmm |
repetitive code is bad |
| 18:39 |
hmmmm |
what is a "txc"? |
| 18:39 |
hmmmm |
you have repetitive code such as "bool use_default_txc = (txc != NULL) ? false : true; ... if (use_default_txc) txc = txc_default;" |
| 18:40 |
RealBadAngel |
f32[24] table of texure coords |
| 18:40 |
hmmmm |
that doesn't make it more clear |
| 18:40 |
hmmmm |
texture coords for what |
| 18:40 |
RealBadAngel |
mesh faces |
| 18:40 |
hmmmm |
i know what it is, but other people reading the code casually do not |
| 18:40 |
hmmmm |
"txc" is pretty useless to anybody but you yourself |
| 18:40 |
RealBadAngel |
grep for txc |
| 18:40 |
hmmmm |
don't care |
| 18:40 |
hmmmm |
it's not descriptive at all |
| 18:40 |
RealBadAngel |
its used 50+ times in source |
| 18:41 |
RealBadAngel |
if not more |
| 18:41 |
hmmmm |
the minute it's being used as a parameter you need to put in some effort into describing it better |
| 18:41 |
hmmmm |
also just because it's been done before doesn't mean it's right |
| 18:41 |
hmmmm |
old code has buffer overflows everywhere and no security |
| 18:41 |
hmmmm |
because it's been done a lot, I guess that must mean it's the right thing to do, right? |
| 18:42 |
|
Miner_48er joined #minetest-dev |
| 18:42 |
hmmmm |
look, over 100 people jumped off that cliff. it must be the right thing to do! |
| 18:44 |
RealBadAngel |
https://github.com/RealBadAngel/minetestHD/blob/selection_fixes/src/mesh.cpp#L455 |
| 18:44 |
hmmmm |
don't care |
| 18:44 |
RealBadAngel |
fuck, its described there whats that |
| 18:44 |
hmmmm |
as it stands, the name "txc" is useless to everybody but yourself |
| 18:44 |
RealBadAngel |
are you blind or what? |
| 18:44 |
RealBadAngel |
its the code you are commenting |
| 18:45 |
RealBadAngel |
also you would like to put there: https://github.com/RealBadAngel/minetestHD/blob/selection_fixes/src/mesh.cpp#L487 |
| 18:46 |
RealBadAngel |
face_texture_coordinates instead of txc? |
| 18:47 |
RealBadAngel |
sorry man, but you are talkin bullshit now. |
| 18:47 |
hmmmm |
face_texture_coords would be best imho |
| 18:47 |
RealBadAngel |
hmmm lets try |
| 18:47 |
hmmmm |
let me guess, you think short, indecipherable jibberish is more "l33t hax0r" like |
| 18:47 |
hmmmm |
and therefore cooler |
| 18:48 |
Obani |
Is this about a naming convention ? |
| 18:48 |
hmmmm |
maybe it is cooler, but save it for your own projects. seriously. |
| 18:48 |
RealBadAngel |
http://pastie.org/10712563 |
| 18:49 |
RealBadAngel |
here you go |
| 18:49 |
RealBadAngel |
happy reading and formatting such piece :P |
| 18:49 |
hmmmm |
you're being obtuse on purpose |
| 18:49 |
Obani |
lol |
| 18:49 |
Obani |
I think face_texture_coords is too long |
| 18:49 |
hmmmm |
float *txc = face_texture_coords; |
| 18:50 |
Obani |
but txc is impossible too hard to understand/memorize |
| 18:50 |
red-001 |
texture_coords? |
| 18:50 |
hmmmm |
you act like there is some kind of direct size/understandability tradeoff without even considering alternative solutions |
| 18:51 |
RealBadAngel |
i think you are just overreacting on such simple things |
| 18:51 |
hmmmm |
and I can say the same of you |
| 18:51 |
hmmmm |
if it's such a simple thing, why not just make me happy and change it? |
| 18:51 |
RealBadAngel |
why should i change something which is kinda convention when it comes to meshes? |
| 18:52 |
hmmmm |
just rename the parameter "face_texture_coords", and add an f32 *txc = face_texture_coords; right before the loop |
| 18:52 |
hmmmm |
because it's apparently a big deal |
| 18:52 |
RealBadAngel |
txc, tsrc, shdrscr, BS, etc |
| 18:52 |
red-001 |
RBA did you fix the build error in VS2013 yet? |
| 18:52 |
hmmmm |
you call it such a simple thing, but here you are making it a big deal |
| 18:52 |
RealBadAngel |
we are using many common shortcuts there |
| 18:52 |
Obani |
vhdjsbfjsdbf |
| 18:52 |
Obani |
I like that shortcut |
| 18:53 |
RealBadAngel |
and all the gfx related functions are using the same ones |
| 18:53 |
hmmmm |
[01:42 PM] <hmmmm> look, over 100 people jumped off that cliff. it must be the right thing to do! |
| 18:53 |
RealBadAngel |
look, now i dont have to think how the stuff is called elsewhere |
| 18:54 |
RealBadAngel |
because i do know the convention |
| 18:54 |
RealBadAngel |
i type tsrc, txc and i know it will be there |
| 18:54 |
hmmmm |
right |
| 18:55 |
hmmmm |
you're coding it for YOURSELF |
| 18:55 |
RealBadAngel |
and you want me to change the whole codebase convention because i used it in a single call |
| 18:55 |
RealBadAngel |
huh? |
| 18:55 |
RealBadAngel |
its not my invention :P |
| 18:55 |
hmmmm |
no, I just want you to change the parameter name in that single function |
| 18:55 |
hmmmm |
that's all |
| 18:55 |
hmmmm |
you're saying - |
| 18:55 |
RealBadAngel |
i just got used to it |
| 18:55 |
|
EvergreenTree joined #minetest-dev |
| 18:55 |
hmmmm |
"I know the convention, I know it will be there, I know this, I know that" |
| 18:55 |
hmmmm |
did you ever think about future coders working on the graphics? |
| 18:56 |
|
Miner_48er joined #minetest-dev |
| 18:56 |
hmmmm |
look I realize that you like to pretend that you're the only one who can work on graphics but seriously cut the shit |
| 18:56 |
RealBadAngel |
yes, they will read comment: //Compute texture coords |
| 18:56 |
hmmmm |
texture coords for what |
| 18:56 |
hmmmm |
also I don't get why you need a comment when you can simply name the parameter something sane in the first place |
| 18:56 |
hmmmm |
also what are the chances of them running across that SPECIFIC comment? |
| 18:57 |
hmmmm |
more description is better than less |
| 18:57 |
hmmmm |
stop writing cryptic code. |
| 18:57 |
est31 |
well if something naming related is used all over the codebase then it should be consistent shouldnt it |
| 18:57 |
est31 |
or we could do a big s/txc/texture_coords/g kind of thing |
| 18:58 |
hmmmm |
those are all internal to the functions, here RBA is passing a parameter to a public function |
| 18:58 |
RealBadAngel |
ok, i will comment that call |
| 18:58 |
hmmmm |
oh my god |
| 18:58 |
hmmmm |
why do you need to add a comment |
| 18:58 |
est31 |
*** Error in `bin/minetest': double free or corruption (fasttop): 0x00000000024accc0 *** |
| 18:58 |
hmmmm |
what's actually wrong with my suggestion? |
| 18:58 |
est31 |
interesting |
| 18:58 |
est31 |
but not reproducible |
| 19:02 |
RealBadAngel |
est31, you can ofc do that with such variables, but you will end with lines this: video::S3DVertex(min.X,min.Y,max.Z, 0,-1,0, c, face_texture_coordinates[4],face_texture_coordinates[7]), |
| 19:02 |
RealBadAngel |
repeated 24 times |
| 19:02 |
RealBadAngel |
also look, theres "c" here, wonder whats that ;) |
| 19:04 |
hmmmm |
do you see "c" as the parameter name anywhere? |
| 19:04 |
hmmmm |
or how about something more descriptive like thing_color |
| 19:04 |
hmmmm |
c is fine if it's a short-lived local variable where the meaning is obvious |
| 19:04 |
hmmmm |
the second you make it into the parameter of a public function, you're defining it as part of an external INTERFACE |
| 19:05 |
hmmmm |
you have a duty to make it a good, clean, understandable one |
| 19:06 |
|
Miner_48er joined #minetest-dev |
| 19:06 |
RealBadAngel |
ok, i can make parameter a bit more descriptive |
| 19:07 |
hmmmm |
that's all I asked for |
| 19:07 |
est31 |
oops sorry nrzkt didn't see your comment |
| 19:07 |
RealBadAngel |
but dont expect to exaplain in its name that is a uv texture coordinate for each face of used mesh |
| 19:07 |
est31 |
do you mind if I implement what you asked? |
| 19:07 |
RealBadAngel |
unless you want to copy what i wrote and replace spaces with underscores |
| 19:08 |
RealBadAngel |
but then propably i should also mention what uv mapping is ;) |
| 19:09 |
red-001 |
shouldn't torches be made floodable? |
| 19:09 |
kaadmy |
red-001: aren't they already? |
| 19:09 |
RealBadAngel |
they should imho |
| 19:09 |
red-001 |
kaadmy afaik no |
| 19:10 |
red-001 |
no they are not |
| 19:10 |
RealBadAngel |
unless something changed you can still place torch underwater and it will burn ;) |
| 19:12 |
red-001 |
fire should be floodable |
| 19:12 |
red-001 |
would make the code a lot smaller |
| 19:13 |
red-001 |
maybe an abm could be removed |
| 19:14 |
RealBadAngel |
ok, i will dig through 3662 now with all the comments |
| 19:16 |
RealBadAngel |
meanwhile #3612 doesnt require any further changes i suppose? |
| 19:16 |
ShadowBot |
https://github.com/minetest/minetest/issues/3612 -- Filmic HDR tone mapping by RealBadAngel |
| 19:16 |
red-001 |
ohh |
| 19:16 |
red-001 |
floodable doesn't drop the node |
| 19:17 |
red-001 |
and doesn't call a callback |
| 19:24 |
red-001 |
that's bad |
| 19:29 |
est31 |
nrzkt, you want to have a look https://github.com/est31/minetest/commit/2f1c8794e0d3239284eaafce69a25efc0e7ccf1e |
| 19:29 |
|
Krock joined #minetest-dev |
| 19:29 |
est31 |
I did what you asked in #3610 |
| 19:30 |
ShadowBot |
https://github.com/minetest/minetest/issues/3610 -- Use meshes to display inventory items by RealBadAngel |
| 19:33 |
|
Amaz joined #minetest-dev |
| 19:33 |
Fixer |
RealBadAngel, i'm at this very moment making a simple MT benchmark with tinytask tool that records mouse and keyboard, fraps and special world and config, will see how it goes :} |
| 19:34 |
Fixer |
now i can also measure reliably that horrible mapgen or meshgen stutter |
| 19:34 |
Fixer |
it will reproduce exact path and actions |
| 19:35 |
RealBadAngel |
est31, static bool enable_animations |
| 19:36 |
RealBadAngel |
whoops, wont work |
| 19:36 |
est31 |
RealBadAngel, what about it? |
| 19:36 |
RealBadAngel |
hmm, wait a sec |
| 19:37 |
RealBadAngel |
ive tried already to make it static or const, one of them not worked |
| 19:37 |
RealBadAngel |
i mean it works just once |
| 19:37 |
RealBadAngel |
then changing a setting in main menu wont help |
| 19:39 |
RealBadAngel |
just try goin back to main menu, changing the setting and back to game |
| 19:39 |
est31 |
nrzkt, I have linked the commit : https://github.com/est31/minetest/commit/2f1c8794e0d3239284eaafce69a25efc0e7ccf1e |
| 19:39 |
est31 |
nrzkt, merge it if you +1 it, or if you dont want to merge, +1 on github, or in IRC |
| 19:39 |
est31 |
will monitor all of them |
| 19:39 |
est31 |
bye! |
| 19:49 |
|
LuckyLeaf joined #minetest-dev |
| 19:52 |
|
LuckyLeaf left #minetest-dev |
| 20:02 |
|
Darcidride joined #minetest-dev |
| 20:04 |
|
nrzkt joined #minetest-dev |
| 20:04 |
nrzkt |
est31: i like your change |
| 20:05 |
RealBadAngel |
a bit overcomplicated imho but nice |
| 20:08 |
RealBadAngel |
i think that "rotation_kind" is kinda confusing |
| 20:09 |
RealBadAngel |
as if dragged or hovered were animated different way |
| 20:10 |
RealBadAngel |
those flags were only to pass which one is displayed to store its mesh and update timers, not animate them different ways |
| 20:12 |
red-001 |
game#829 |
| 20:12 |
ShadowBot |
https://github.com/minetest/minetest_game/issues/829 -- Make grass, flowers, fire, crops and torchs floodable. by red-001 |
| 20:12 |
red-001 |
not sure if making torches floodable is a good idea |
| 20:13 |
RealBadAngel |
or torchs ? |
| 20:13 |
|
Gael-de-Sailly joined #minetest-dev |
| 20:13 |
RealBadAngel |
^^typo in commit title |
| 20:13 |
red-001 |
thanks |
| 20:16 |
red-001 |
fixed |
| 20:24 |
red-001 |
#3667 |
| 20:25 |
ShadowBot |
https://github.com/minetest/minetest/issues/3667 -- Sneak can be used to stop a fall without fall damage. |
| 20:43 |
|
Krock joined #minetest-dev |
| 20:57 |
|
jordan4ibanez joined #minetest-dev |
| 21:51 |
|
troller joined #minetest-dev |
| 21:51 |
|
paramat joined #minetest-dev |
| 21:53 |
|
proller__ joined #minetest-dev |
| 22:04 |
|
kaadmy joined #minetest-dev |
| 23:21 |
|
rubenwardy_ joined #minetest-dev |
| 23:32 |
|
est31 joined #minetest-dev |
| 23:34 |
est31 |
I'll merge the commit now with hmmmm 's proposed adjustments |
| 23:34 |
hmmmm |
what?? |
| 23:34 |
est31 |
first value in enum is 0 |
| 23:34 |
hmmmm |
oh |
| 23:35 |
hmmmm |
yeah everything looks fine |
| 23:35 |
hmmmm |
that's the only thing i could nitpick about |
| 23:35 |
hmmmm |
there's no issue with being more explicit either, it's just that i figured you might not be aware of that rule |
| 23:35 |
hmmmm |
that enums always start at 0 |
| 23:36 |
est31 |
pushed |
| 23:37 |
est31 |
well I wasn't sure, and I saw other code do it |
| 23:37 |
est31 |
codebase is bad teacher :) |
| 23:37 |
hmmmm |
it's a good teacher to learn quickly, but at some point you should start reading the language spec |
| 23:59 |
paramat |
RealBadAngel if you haven't seen this yet #3668 |
| 23:59 |
ShadowBot |
https://github.com/minetest/minetest/issues/3668 -- Wield images are set as inventory images since 6cd2b3b |
| 23:59 |
|
_rubenwardy_ joined #minetest-dev |
