| Time |
Nick |
Message |
| 00:02 |
|
Tmanyo joined #minetest-dev |
| 00:07 |
Fixer |
thats why minetest-dev in dissaray, someone wants to merge and fix, someone wants nuclear reactor grade software %) |
| 00:08 |
Fixer |
it is so toxic right now |
| 00:14 |
Hijiri |
nuclear reactor grade software probably would need more static verification |
| 00:14 |
Hijiri |
anyway daily pls review post: #4685 |
| 00:14 |
ShadowBot |
https://github.com/minetest/minetest/issues/4685 -- Add control information to player interacts by raymoo |
| 00:14 |
Hijiri |
don't let the uncertainty of drama prevent you from reviewing |
| 00:16 |
Fixer |
haha, good luck with that |
| 00:16 |
Fixer |
there was 3 pages of hate above |
| 00:16 |
Hijiri |
I was here |
| 00:16 |
Hijiri |
was afk but skimmed it |
| 00:21 |
Fixer |
"oh shit we broke something because we didn't talk it over", "that's okay we have another commit" /// but why not? it is not like someone died.. I play another game called CDDA which has exactly this attitude and it is not only playable and gives you lots of fun, game makes huge progress between releases, while minetest adds (?) carts after 5 years of waiting %) |
| 00:21 |
Fixer |
nobody is perfect anyway, including coders |
| 00:22 |
Fixer |
and one thing i dislike about this debate |
| 00:23 |
Fixer |
people bitching about some obscure refactoring, while gameplay or graphics or water or whatever bugs are left out for years for unknown reasons, no drama about that stuff :( |
| 00:24 |
Fixer |
maybe coding requirements are too high for minetest and it is holding development? people afraid to even code smth |
| 00:33 |
Fixer |
it is just a block game... don't ruin it, be good to each other, you will not loose job because of this, nobody will execute you, commit a fix and move... with current attitute minetest in 10 years will hardly move to anything... |
| 00:33 |
Fixer |
good night |
| 00:49 |
rubenwardy |
code review is very important though |
| 00:50 |
rubenwardy |
oh, he went |
| 01:01 |
|
paramat joined #minetest-dev |
| 01:02 |
paramat |
fixer talking rubbish again |
| 01:07 |
paramat |
#4686 seems good now |
| 01:07 |
ShadowBot |
https://github.com/minetest/minetest/issues/4686 -- Remove optimization that causes underwater and cave rendering glitches. by lhofhansl |
| 01:15 |
|
STHGOM joined #minetest-dev |
| 01:30 |
|
turtleman joined #minetest-dev |
| 01:57 |
paramat |
any more approval for 4686? |
| 02:30 |
|
fling joined #minetest-dev |
| 02:41 |
|
Zeno` joined #minetest-dev |
| 03:06 |
|
ptv joined #minetest-dev |
| 03:17 |
|
hiradur joined #minetest-dev |
| 03:43 |
paramat |
game#1358 |
| 03:43 |
ShadowBot |
https://github.com/minetest/minetest_game/issues/1358 -- Add a telescope item that enables zoom privilege |
| 03:48 |
paramat |
good, 4686 is in :] |
| 03:53 |
hmmmmm |
is this the same zoom that kaeza (???) made a PR for years ago and not enough people liked it to get merged? |
| 03:53 |
hmmmmm |
oh nvm it is in |
| 04:19 |
|
Foghrye4_ joined #minetest-dev |
| 04:21 |
paramat |
it was eventually merged in another form, it was made disabled by default and only allowed through privilege |
| 04:32 |
hmmmmm |
yeah that sounds like a good compromise |
| 04:40 |
sofar |
visually correct rendering has my preference, so +1 for 4686 |
| 04:40 |
sofar |
so many good visual changes in the past few days now, wow |
| 04:45 |
paramat |
yes lots of good stuff all around |
| 04:48 |
hmmmmm |
good stuff, but, there's one thing i really don't like, "Added my own approval" |
| 04:51 |
paramat |
heh i noticed :] |
| 04:51 |
hmmmmm |
it's a blatant attempt to subvert the whole point behind peer review |
| 04:52 |
hmmmmm |
sure, you can have your own approval in the sense that you feel confident in the code you write, but that doesn't count for any points |
| 04:53 |
hmmmmm |
but on the other hand... if you don't feel confident about your own code, what kind of message does that send to the rest of us? |
| 04:54 |
paramat |
the current practice by most is that an author's own contentment doesn't count as an 'approval' label |
| 04:54 |
hmmmmm |
i noticed a lot of people like to strawman the push for code quality, comparing it to "nuclear reactor code" and other similar things |
| 04:55 |
hmmmmm |
there's a huuuuge gamut of code quality |
| 04:55 |
paramat |
oh fixer was doing the negativity thing, talking rubbish again |
| 04:55 |
hmmmmm |
we're nowhere near nuclear reactor code quality |
| 04:55 |
hmmmmm |
what i'm hoping for is a level of quality such that a new commit does not add new bugs |
| 04:55 |
paramat |
yeah the required code quality is barely high enough as it is |
| 04:56 |
hmmmmm |
because that's what a lot of these things tend to do |
| 04:56 |
hmmmmm |
shuffle around the old bugs to make new ones |
| 04:56 |
hmmmmm |
how much manpower and effort is put toward solving bugs that exist already? |
| 04:57 |
hmmmmm |
how much does it upset the end users that there are so many bugs? |
| 04:58 |
hmmmmm |
the thing is, the code review stage often misses many flaws |
| 04:59 |
hmmmmm |
even if there are multiple reviewers and they're all extremely careful |
| 04:59 |
hmmmmm |
the earlier a bug is seen, the lower the cost of remediation |
| 05:00 |
paramat |
sofar is setting a higher quality requirement in mtgame, much appreciated |
| 05:00 |
|
proller__ joined #minetest-dev |
| 05:02 |
hmmmmm |
the thing about code reviews |
| 05:03 |
hmmmmm |
i notice a lot of people just visually scan over it and note things that pop out to them, such as a buffer overrun or missing check |
| 05:03 |
hmmmmm |
rarely do reviewers build up the mental state of the entire PR and make note of logical problems the changes introduce |
| 05:05 |
hmmmmm |
i've personally noticed that with a group of decently competent coders, the great majority of bug types are logic errors, not using the wrong function or indexing an array out of bounds or overflowing a stack |
| 05:05 |
hmmmmm |
it's mentally taxing to do this |
| 05:05 |
hmmmmm |
i don't blame more reviewers for not |
| 05:06 |
hmmmmm |
but at the same time it's perhaps an incentive to push harder for real unit tests that verify behavior on the smooth path as well as various edge cases |
| 05:06 |
hmmmmm |
also it's a reason to demand easy to understand code |
| 05:07 |
hmmmmm |
which is a big part of the reason why the shader setting cache PR is being shelved for the immediate time being |
| 05:07 |
hmmmmm |
it's just far too difficult to verify the behavior as being correct with the many levels of indirection |
| 05:08 |
hmmmmm |
so my point is - having a single other person "review" the code by scanning it over and looking for obvious errors is the bare minimum of a review. you could probably get the same or better results from a static code analyzer |
| 05:09 |
hmmmmm |
that's why two people other than yourself looking at it is considered the minimum for a change of moderate size and moderate complexity |
| 05:09 |
hmmmmm |
and those code reviews (plural) themselves shouldn't be enough to feel confident in the code being considered, unit tests should also be there |
| 05:10 |
hmmmmm |
the only reason why i haven't been harping on automated testing much more is because it's difficult to test |
| 05:10 |
hmmmmm |
nothing is broken out into discrete units and refactoring it to do so would simply be a massive undertaking |
| 05:24 |
|
gregorycu joined #minetest-dev |
| 05:25 |
gregorycu |
I don't exactly think it's fair to characterise Fixer's points as rubbish or being negative |
| 05:27 |
paramat |
many in the last few hours have been |
| 05:27 |
gregorycu |
He's been around a while, and often the PRs he cares about fall by the wayside (for whatever reason) |
| 05:29 |
gregorycu |
There are open source projects with more stringent requirements for merging, and also those with less stringent requirements. He thinks minetest is too stringent . |
| 05:35 |
sofar |
We all have different values, and they just conflict |
| 05:39 |
paramat |
'nobody fixes bugs' 'nobody cares about bugs' 'no drama about bugs' 'people afraid to even code something' fixer often does these ridiculous negative rants and exaggerates for effect, it's ok i'm used to it now, fixer is also a valued contributor :] |
| 05:40 |
gregorycu |
I think they are true though |
| 05:41 |
gregorycu |
Maybe a slight exaggeration |
| 05:41 |
gregorycu |
And most of it is a function that minetest is an open source project |
| 05:42 |
paramat |
we worrk a |
| 05:43 |
paramat |
our asses off worrying about bugs, fixing them, discussing them. the actual issue is lack of dev time, which is what fixer needs to understand |
| 05:43 |
paramat |
oops |
| 05:44 |
gregorycu |
Lack of dev time is AN issue, agree |
| 05:44 |
gregorycu |
I'm sure he understands it |
| 05:47 |
paramat |
yeah, it's just not expressed during a rant |
| 05:47 |
gregorycu |
Yeah |
| 05:51 |
|
jin_xi joined #minetest-dev |
| 05:57 |
|
Hunterz joined #minetest-dev |
| 06:06 |
|
nrzkt joined #minetest-dev |
| 06:43 |
|
CWz joined #minetest-dev |
| 07:14 |
|
kaeza joined #minetest-dev |
| 07:54 |
|
CWz_ joined #minetest-dev |
| 08:15 |
paramat |
added docs to #4699 and will merge later |
| 08:15 |
ShadowBot |
https://github.com/minetest/minetest/issues/4699 -- Lua voxelmanip: Add optional buffer param for 'get param2 data' by paramat |
| 08:31 |
paramat |
tested too |
| 08:53 |
|
Hunterz joined #minetest-dev |
| 09:23 |
|
red-001 joined #minetest-dev |
| 09:34 |
|
paramat joined #minetest-dev |
| 10:16 |
est31 |
I agree with hmmm that reviews should be higher quality, but we don't have enough devs to do it |
| 10:16 |
est31 |
the best way hmmm can make review quality go up is to review himself |
| 10:17 |
est31 |
you know, because i rather introduce a bug than let minetest drown in 200 open PRs |
| 10:17 |
est31 |
and demote potential contributors along the way |
| 10:24 |
paramat |
yes, so an author's happiness with their PR counts as one of the 2 core dev agreements required |
| 10:27 |
paramat |
(er as long as they are a core dev) |
| 10:27 |
paramat |
that is also the minimum acceptable amount of review |
| 10:43 |
|
Fixer joined #minetest-dev |
| 10:50 |
paramat |
will merge #4681 #4699 in a moment |
| 10:50 |
ShadowBot |
https://github.com/minetest/minetest/issues/4681 -- Replace call to obsolete get_look_pitch(). by sofar |
| 10:50 |
ShadowBot |
https://github.com/minetest/minetest/issues/4699 -- Lua voxelmanip: Add optional buffer param for 'get param2 data' by paramat |
| 10:52 |
nrzkt |
paramat, est31 #4155 is now ready and lighter (i removed the DB backend atm as it's not needed for the part 1 of this PR, this permit easy review/integration) |
| 10:52 |
ShadowBot |
https://github.com/minetest/minetest/issues/4155 -- Implement player attribute backend by nerzhul |
| 11:02 |
paramat |
now merging |
| 11:02 |
est31 |
file backends are broken |
| 11:02 |
est31 |
we should use databases, that's what they are made for |
| 11:03 |
est31 |
many minetest admins regularly have to fix broken player files |
| 11:03 |
est31 |
plain text files are fine AS LONG AS WE ARE NOT WRITING TO THEM |
| 11:05 |
paramat |
merge complete |
| 11:05 |
nrzkt |
est31, i agree with you for that |
| 11:05 |
nrzkt |
but i should do the PR in two phasis atm |
| 11:06 |
nrzkt |
because the db backend will be a little bit more huge PR because of our list of backends |
| 11:06 |
nrzkt |
i already have the design to do it in pg & sqlite, others are not ready |
| 11:07 |
nrzkt |
and sorry est31 but the purpose of the PR is not to rewrite the player file, just add a feature to players |
| 11:07 |
nrzkt |
i remove your label as it's not adapted to its case |
| 11:07 |
est31 |
no, it should remove the player file completely |
| 11:07 |
nrzkt |
then merge this and let me finish the second part |
| 11:07 |
est31 |
and add a migration system |
| 11:07 |
est31 |
no |
| 11:08 |
nrzkt |
i don't want to do a huge PR and hmmm will cry if we remove files, as you know |
| 11:08 |
est31 |
finish it, then we can merge the combined pr |
| 11:08 |
est31 |
we should remove them, sorry |
| 11:08 |
est31 |
they are horribly broken |
| 11:09 |
nrzkt |
paramat it's not controversial PR, the feature is not controversial, it only miss a db backend xD |
| 11:09 |
est31 |
you know this when server admins get SPEEDUPs when removing player files |
| 11:09 |
nrzkt |
est31, i agree with you |
| 11:09 |
gregorycu |
Does minetest need to be rewritten? |
| 11:09 |
nrzkt |
but wait a minute, each change at a moment |
| 11:10 |
nrzkt |
i already have a full postgresql + sqlite3 player backend on my server + the migration process written in python, but it's not the purpose of this PR |
| 11:10 |
nrzkt |
this PR only adds extended attributes on palyer,s not rewrite the player backend |
| 11:10 |
nrzkt |
it's another PR |
| 11:10 |
est31 |
nrzkt: its okay if you only add extended attributes for now |
| 11:10 |
nrzkt |
just let the two features separated |
| 11:10 |
est31 |
but do not store them in json files |
| 11:10 |
est31 |
not at all |
| 11:10 |
nrzkt |
est31, it's the only thing this PR does |
| 11:10 |
est31 |
no json files |
| 11:10 |
nrzkt |
no jsonfiles here |
| 11:11 |
est31 |
store them in databases |
| 11:11 |
nrzkt |
it's an attribute in the player file |
| 11:11 |
est31 |
ah |
| 11:11 |
est31 |
no to that as well |
| 11:11 |
est31 |
we should not extend the broken system |
| 11:11 |
nrzkt |
but let the two things separated, i need time to finish to write the second part :) |
| 11:11 |
est31 |
the bigger a player file, the more time it will take to load it |
| 11:11 |
est31 |
and to write it |
| 11:11 |
nrzkt |
see this as a ... feature preview :) |
| 11:12 |
gregorycu |
Do you want my opinion on the matter? |
| 11:12 |
gregorycu |
Does this change add a new datapoint, or does it make potential future migrations to db backend harder? |
| 11:12 |
nrzkt |
no |
| 11:13 |
nrzkt |
because it only add an attribute and i already have a working solution for this in relationnal DB which is wrote in another branch |
| 11:13 |
gregorycu |
If it doesn't make it harder to refactor in the future... |
| 11:13 |
nrzkt |
look at the result (i don't write the syntax of extended attribute, just the structure |
| 11:13 |
nrzkt |
http://pastebin.com/3JALxy37 |
| 11:13 |
nrzkt |
oops i pasted two outputs, but the same :) |
| 11:14 |
est31 |
okay, I've read the changes, still -1 |
| 11:14 |
nrzkt |
why ? except this is not stored in DB ? |
| 11:14 |
est31 |
we can make that change once we store player data in the database |
| 11:15 |
est31 |
but the bigger we make player files, the more likely they break |
| 11:15 |
nrzkt |
you know, if i add the DB storage backend now i see at many kilometers that kwoelr will complain about the complexity of the PR and it's better to do kiss and working commits and he wants a file storage :p |
| 11:15 |
est31 |
plus, we write player files each few seconds |
| 11:15 |
nrzkt |
est31, yes, but we will write to DB each time too |
| 11:15 |
est31 |
that doesnt have to be |
| 11:16 |
est31 |
just write to the db each time you change something |
| 11:16 |
nrzkt |
it's what we do with player files in fact |
| 11:16 |
nrzkt |
the dirty flag on remote player is for that case |
| 11:16 |
nrzkt |
we save if dirty flag is set or player disconnects |
| 11:17 |
est31 |
yes |
| 11:17 |
est31 |
but you store all attributes for a player |
| 11:17 |
est31 |
not just those that changed |
| 11:18 |
est31 |
if mods start using the system for all kinds of things this becomes a problem |
| 11:18 |
est31 |
right now the system is only used for things that change often anyway |
| 11:18 |
est31 |
like inventory or position |
| 11:18 |
est31 |
but they are few things |
| 11:18 |
est31 |
and very litle data for each |
| 11:18 |
nrzkt |
yes, with relationnal DB we should be a little bit precise on what we save (inventory, player, extended attributes), but write 1kb data vs write to DB doesn't change the load on the server, but yes a DB is better for server owners to make a website interact with it |
| 11:19 |
est31 |
but what happens when mods add many things and big data each |
| 11:19 |
nrzkt |
in fact position changes everytime |
| 11:19 |
est31 |
yes, you must assume that you rewrite the files for each emerged player |
| 11:20 |
nrzkt |
est31, if mods store too many datas, storing in DB vs storing in files doesn't change anything, you write same or greater amount of data |
| 11:20 |
est31 |
it depends on how you make your DB |
| 11:21 |
est31 |
if you make your DB with one column for the player name (PRIMARY KEY) one for inventory one for position and one for extended attributes |
| 11:21 |
nrzkt |
lol |
| 11:21 |
nrzkt |
wrong design |
| 11:21 |
est31 |
then you indeed have to rewrite everything each time |
| 11:21 |
nrzkt |
inventory should have its own table with (inventory_name, slot_id, item_id, item_count |
| 11:21 |
est31 |
but if you make your DB with one column for the player name and one for the attribute name (they two combined make the PRIMARY KEY) |
| 11:21 |
est31 |
then its okay |
| 11:22 |
est31 |
nrzkt: that's overengineered IMO |
| 11:22 |
nrzkt |
no est31 because this permits to dump easily the inventory o na website |
| 11:22 |
nrzkt |
and it's very cheap to save |
| 11:23 |
est31 |
you store more IDs and stuff than actual inventory data |
| 11:23 |
nrzkt |
i have this on my server, let me a minute to give you stats |
| 11:23 |
nrzkt |
not a problem for a relational db |
| 11:23 |
est31 |
nrzkt: that should not be the measure we optimize against |
| 11:23 |
est31 |
nrzkt: what do you propose next, storing each node separately in the db? |
| 11:23 |
nrzkt |
if you use a relationnal DB to do NoSQL DB it's totally useless :) |
| 11:24 |
nrzkt |
then use mongodb |
| 11:24 |
est31 |
so your argument is "because we can use it we should"? |
| 11:24 |
nrzkt |
i have 567849 stored in my db |
| 11:25 |
est31 |
even though its worse? |
| 11:25 |
nrzkt |
and 3077454 |
| 11:25 |
nrzkt |
items |
| 11:25 |
est31 |
you just need to write some little deserialisation code on the webserver, that shouldn't be too hard should it? |
| 11:25 |
nrzkt |
if you want a DB USE the DB, not only store data because it's a DB |
| 11:26 |
nrzkt |
a website should read the properties directly, not doing deserialization, don't forget server owners are mainly using Lua, they don't want to deserialize but have a properly designed object |
| 11:27 |
nrzkt |
if you want a db backend then i propose to do a DB PR first, merge it and then i will rebase and fix the current PR to use it |
| 11:27 |
nrzkt |
not merge the two features in one PR |
| 11:27 |
est31 |
well we can provide deserialisation libraries in python and in ruby |
| 11:27 |
est31 |
then server owners can use those for their websites |
| 11:28 |
est31 |
or if you want in php too |
| 11:28 |
nrzkt |
est31, and who maintain those libraries ? us ? |
| 11:28 |
est31 |
nrzkt: that proposal sounds good |
| 11:28 |
nrzkt |
sorry but giving a proper relationnal DB because we are using it vs: adding overhead server & client side is not the good solution |
| 11:29 |
est31 |
we can do it in two prs |
| 11:29 |
est31 |
but this PR is blocked |
| 11:30 |
nrzkt |
you blocked, if two others wants it this doesn't block anything. But okay for adding DB before merging that if you want |
| 11:30 |
Calinou |
<gregorycu> Does minetest need to be rewritten? |
| 11:30 |
est31 |
why should it |
| 11:30 |
Calinou |
maybe, every time I talk about the world storage issue, people tell me "write your own binary format" |
| 11:30 |
Calinou |
"and don't use relational SQL databases for this" |
| 11:31 |
gregorycu |
I have a feeling that both non-immediate graphics, along with client side (predictive) lua, and maybe even SQL databases probably require a ground-zero rewrite of the architecture |
| 11:32 |
est31 |
well maybe someone thinks this is an abuse of the relational db model |
| 11:32 |
est31 |
but we use dbs not for their relational features |
| 11:32 |
est31 |
we use them for their storage features |
| 11:33 |
est31 |
that you can crash and your db still works fine |
| 11:33 |
gregorycu |
For their ACID? |
| 11:33 |
est31 |
gregorycu: yes |
| 11:34 |
est31 |
also, if you use many little files there is this second issue that filesystems weren't really designed for that case |
| 11:35 |
est31 |
like the fact that a fs always allocates at least one hdd block |
| 11:35 |
est31 |
and that some filesystems are case insensitive |
| 11:36 |
gregorycu |
And some filesystems have characters that are illegal |
| 11:36 |
gregorycu |
(Which you may want to use for filenames) |
| 11:36 |
Zeno` |
what's ground-zero mean? |
| 11:37 |
est31 |
https://github.com/nerzhul/minetest/blob/master/src/player.cpp#L409 |
| 11:37 |
est31 |
just look at this |
| 11:38 |
gregorycu |
est31: Yeah, it's retarded. I'm not sure nrzkt disagrees with us however. |
| 11:38 |
est31 |
he agrees I think |
| 11:38 |
gregorycu |
Zeno`: Bottom up rewrite |
| 11:39 |
est31 |
I do disagree with him about making everything totally relational though |
| 11:39 |
Zeno` |
hmm ok. I'd never heard the term ground zero used that way before |
| 11:39 |
gregorycu |
I'm special |
| 11:42 |
nrzkt |
yees est31 this code is an aberation, i didn't touch it because i didn't know the side effects :p |
| 11:43 |
nrzkt |
est31, if i have a website and i want an inventory based on its name or find all items of a player based on item name, i really should load all the inventory data to do an update or a delete ? this is overkill |
| 11:44 |
gregorycu |
nrzkt: I suppose this is a question of usecase |
| 11:44 |
est31 |
yes |
| 11:44 |
est31 |
and, for what usecase do you need to do this kind of thing |
| 11:44 |
paramat |
website linkage is low priority |
| 11:45 |
nrzkt |
gregorycu, yes, but having serialization doesn't permit this usecase, and to do stats or massive removal of an item into db (if node name changed or was removed by the admin) |
| 11:45 |
est31 |
nrzkt: that's a valid point |
| 11:46 |
gregorycu |
Feels like hyper-normalisation to me |
| 11:46 |
est31 |
although removals can be handled by removing it when loading the inventory |
| 11:46 |
est31 |
and stats by nightly run scripts |
| 11:47 |
est31 |
but I admit both those approaches are worse than some sql statement .) |
| 11:47 |
est31 |
still, the removal problem needs to be solved by the map as well |
| 11:48 |
est31 |
and now excuse me, I've got to do some stuff for uni today |
| 11:48 |
* est31 |
walks away |
| 11:54 |
nrzkt |
yes we can fix it when loading inventory, but core didn't know better than admin what to change |
| 11:54 |
nrzkt |
interesting discussion anyway |
| 12:18 |
|
Wuzzy joined #minetest-dev |
| 12:18 |
Wuzzy |
hi all |
| 12:19 |
Wuzzy |
hi paramat |
| 12:19 |
Wuzzy |
I want to talk about the workflow on GitHub |
| 12:20 |
nrzkt |
yes ? |
| 12:21 |
Wuzzy |
i dont mean to offend or insult me, but why is it you just dont commit the changes you request from a PR yourselves? in PR 1357, you seem to have already done the work, but you are still asking me to basically repeat what you have already did. Why? Personally, I find this practice pretty annoying |
| 12:21 |
Wuzzy |
This goes mostly to paramat |
| 12:22 |
Wuzzy |
oh, in case you missed it: GitHub has now a feature called “Allow edits from maintainers.â€. you can directly edit on a branch of a PR when you're maintainer |
| 12:22 |
Zeno` |
#1357 |
| 12:22 |
ShadowBot |
https://github.com/minetest/minetest/issues/1357 -- Allow custom liquids to have drops by sfan5 |
| 12:23 |
Wuzzy |
no, this is minetest_game. |
| 12:23 |
Wuzzy |
sorry |
| 12:23 |
Zeno` |
oh :) |
| 12:23 |
Wuzzy |
oops I mean Minetest Game |
| 12:26 |
Wuzzy |
hmmm paramat is not there, it seems. :( |
| 12:27 |
Wuzzy |
i hate it when in IRC people seem to be always “there†24/7 while, in fact, they're not actually there ... lol |
| 12:27 |
paramat |
you wrote you could not do the texture yourself, sofar suggested the devs do it instead, so to simplify things and speed up the process i did the texture myself and said you can use it in the PR if you want |
| 12:27 |
paramat |
it's an all-round good thing i did |
| 12:28 |
paramat |
it's also normal practice for devs to ask for changes to a PR, to be done by the author, instead of doing the work themselves |
| 12:29 |
paramat |
you have got annoyed by this before, it's weird |
| 12:33 |
red-001 |
opinions on #4642 ? |
| 12:33 |
ShadowBot |
https://github.com/minetest/minetest/issues/4642 -- Allow the join/leave message to be overridden by mods. by red-001 |
| 12:33 |
Wuzzy |
paramat, it is not about that you're asking for changes in PRs. i mean, if you found a serious bug you have good reason to expect from the submitter to fix it |
| 12:34 |
Wuzzy |
its just that youre asking me to do something what you apparently already did. It's weird. Maybe I just misunderstood something. idk |
| 12:38 |
paramat |
well, i just made a texture you could use, this way you save us having to alter your commit or add another commit, after all you are the contributor and it's your PR, you're expected to do the work |
| 12:39 |
Wuzzy |
did you actually touch the code or how did you make the screenshot? |
| 12:39 |
paramat |
thanks for using it |
| 12:39 |
paramat |
and for updating the PR, appreciated |
| 12:40 |
paramat |
i just made the texture and put it into my local copy of mtgame to test it, i didn't go near your PR |
| 12:40 |
Wuzzy |
i will talk to you later. |
| 12:40 |
Wuzzy |
OOOHhh... |
| 12:40 |
Wuzzy |
i see |
| 12:40 |
paramat |
ah :] |
| 12:40 |
Wuzzy |
in that case, forget my complaint |
| 12:40 |
paramat |
no problem then |
| 12:41 |
Wuzzy |
I thought you have actually edited the code, and then asked me to do the same changes you did, and submit them ... XD |
| 12:41 |
paramat |
i see |
| 12:42 |
paramat |
whenever i ask for changes i rarely touch the commit itself |
| 12:47 |
Wuzzy |
ok |
| 12:48 |
Wuzzy |
paramat: Out of curiosity: What do you think of the “Allow edits by maintainers†feature in general? Is it good or will you more tend to avoid it? (btw: this question goes out to other Minetest and Minetest Game devs) |
| 12:49 |
paramat |
that may have been the misunderstanding last time too :] anyway cool .. |
| 12:50 |
paramat |
erm i haven't dared use that, i'm hesitant about these new github features |
| 12:50 |
Wuzzy |
i can kinda understand why |
| 12:50 |
Wuzzy |
but the PR submitter can always opt to disable it |
| 12:50 |
paramat |
i tend to work in terminal with git as much as possible and avoid fancy github stuff |
| 12:51 |
Wuzzy |
lol ok :D |
| 12:51 |
Wuzzy |
hmmm idk maybe this feature also implies that it grants maintainers commit access to the PR branch. regardless of GitHub or commandline. i havent tested it yet |
| 12:52 |
paramat |
i actually haven't thought about 'allow edits by' at all, don't even understand it |
| 12:52 |
paramat |
i doubt there is access to your branch, just edits to the commit |
| 12:52 |
Wuzzy |
as far I understand it, it means that maintainers of the repository to which the PR is posted to (minetest_game in this case) can directly commit to the branch of the PR |
| 12:53 |
Wuzzy |
wait, what? *editing* commits? if this is true it this would be very wrong |
| 12:53 |
paramat |
i hope not |
| 12:53 |
est31 |
yes that is my understanding too |
| 12:53 |
est31 |
(I'm semi present) |
| 12:53 |
Wuzzy |
ah, i see. you want to avoid features you dont fully understand. makes sense :D |
| 12:54 |
Wuzzy |
OK, as far I understand it, as long I have PRs for Minetest or Minetest Game, i should always expecting to see more change requests. And I guess this won't change in the near future. |
| 12:55 |
paramat |
i mean i hope the feature doesn't allow devs to edit a contributor's branch |
| 12:55 |
paramat |
maybe it does .. |
| 12:55 |
nore |
Well, best way to test how it works is to enable it for some test PR and see what happens |
| 12:56 |
Wuzzy |
hmm, i bet GitHub has some help page on this new “fancy†feature.. lemme see |
| 12:56 |
paramat |
anyway, yeah, if you submit a PR unfortunately it can be a long frustrating series of requests from devs for changes |
| 12:56 |
Wuzzy |
I don't want to pollute your repos with “Test PRsâ€. lol |
| 12:57 |
paramat |
1 test PR is ok |
| 12:57 |
Wuzzy |
hmm, what about this: I post PR, wait for a week for requests to pour in, then start editing everything at once. xD |
| 12:57 |
nore |
paramat: yeah, that's why allowing maintainers to edit the PR to fox a few style issues could be useful |
| 12:58 |
paramat |
i see |
| 12:58 |
nore |
since that kind of change is asked quite often, and it is a bit annoying to the one who opened the pr |
| 13:01 |
Wuzzy |
Welcome in my world. XD |
| 13:01 |
Wuzzy |
i try to abide by coding style guide from now on, however |
| 13:01 |
Wuzzy |
"do it right the first time" |
| 13:01 |
Wuzzy |
i just didnt know that even Minetest Game has coding style |
| 13:02 |
nore |
I just read the help page, and how I understand it is that it allows push access by maintainers to the branch of the pull request |
| 13:02 |
Wuzzy |
thx |
| 13:02 |
nore |
So that can include editing commits |
| 13:03 |
Wuzzy |
oh right. by git rebase, and then git push... O SHI- |
| 13:04 |
Wuzzy |
but doing rebasing like this is evil anyway :) |
| 13:08 |
paramat |
if i alter a commit i do it from git in terminal, that way your branch isn't accessed but you still remain the author |
| 13:09 |
paramat |
so there's another way for devs to alter |
| 13:11 |
Wuzzy |
I also use only Git in terminal for my commits |
| 13:11 |
Wuzzy |
By the way: https://github.com/minetest/minetest_game/pull/1346 |
| 13:15 |
paramat |
+1 |
| 13:19 |
|
AcidNinjaFWHR joined #minetest-dev |
| 13:20 |
|
Taoki joined #minetest-dev |
| 13:23 |
|
paramat left #minetest-dev |
| 13:32 |
|
Samson1 joined #minetest-dev |
| 13:34 |
|
STHGOM joined #minetest-dev |
| 13:53 |
|
Taoki joined #minetest-dev |
| 14:57 |
|
garywhite joined #minetest-dev |
| 14:59 |
|
fapper joined #minetest-dev |
| 15:08 |
|
hmmmm joined #minetest-dev |
| 15:17 |
|
Foghrye4_ joined #minetest-dev |
| 15:28 |
|
garywhite_ joined #minetest-dev |
| 15:35 |
|
AcidNinjaFWHR joined #minetest-dev |
| 15:36 |
|
Taoki joined #minetest-dev |
| 16:07 |
|
twoelk joined #minetest-dev |
| 16:35 |
|
jin_xi joined #minetest-dev |
| 16:57 |
|
Taoki joined #minetest-dev |
| 16:58 |
|
Krock joined #minetest-dev |
| 17:10 |
|
Taoki joined #minetest-dev |
| 17:52 |
|
johnnyjoy joined #minetest-dev |
| 17:55 |
|
johnnyjoy joined #minetest-dev |
| 18:11 |
|
Foghrye4__ joined #minetest-dev |
| 18:13 |
|
nrzkt joined #minetest-dev |
| 18:40 |
* sofar |
sees fire, adds oil |
| 18:40 |
Krock |
The school's burning. Get more wood. |
| 18:45 |
jin_xi |
*weed |
| 18:46 |
|
ssieb joined #minetest-dev |
| 18:52 |
|
DI3HARD139 joined #minetest-dev |
| 19:18 |
thePalindrome |
What are the plans for more secure password hashing? |
| 19:18 |
thePalindrome |
I'd like to suggest bcrypt |
| 19:18 |
thePalindrome |
Failing that, anything that's not base64 |
| 19:18 |
Calinou |
thePalindrome: uh, we use SRP with hashing and all |
| 19:18 |
Calinou |
we're not using Base64 for a *long* time now! |
| 19:18 |
Calinou |
with salting too |
| 19:18 |
Calinou |
our password storage is quite secure, secure enough for a game |
| 19:18 |
Calinou |
(this is not a banking application) |
| 19:18 |
thePalindrome |
Ah, the wiki is out of date |
| 19:18 |
Calinou |
I'd still like a "remember password" option though :p |
| 19:19 |
Calinou |
(feel free to complain about its security, but it's up to the user to use it) |
| 19:19 |
thePalindrome |
eh, I can at least get behind that |
| 19:19 |
thePalindrome |
at least the insecurity actually is convienent |
| 19:19 |
thePalindrome |
Could somebody update the wiki? |
| 19:19 |
* thePalindrome |
doesn't have an account :um: |
| 19:19 |
Calinou |
sure, but I need confirmation from ShadowNinja first |
| 19:19 |
Calinou |
(who implemented SRP, IIRC) |
| 19:20 |
thePalindrome |
Which hashing algo? |
| 19:21 |
Calinou |
I don't remember |
| 19:21 |
* thePalindrome |
dives into the code |
| 19:22 |
thePalindrome |
sha-1 it appears |
| 19:24 |
Calinou |
hmm, I wonder why that was used |
| 19:24 |
Calinou |
no dependency on other libraries? |
| 19:24 |
thePalindrome |
Dunno |
| 19:24 |
thePalindrome |
bcrypt? :eager: |
| 19:24 |
hmmmm |
est31 was the one who implemented srp |
| 19:25 |
hmmmm |
pretty sure it's using SHA-256 in compliance with the SRP 6a standard |
| 19:26 |
thePalindrome |
The code seems to be using sha-1 though |
| 19:26 |
thePalindrome |
Unless it's in another branch |
| 19:26 |
hmmmm |
no.... hold on |
| 19:28 |
hmmmm |
https://github.com/minetest/minetest/blob/master/src/util/srp.cpp#L53 |
| 19:28 |
|
Tmanyo joined #minetest-dev |
| 19:29 |
thePalindrome |
Huh |
| 19:29 |
thePalindrome |
auth.cpp uses sha-1 |
| 19:29 |
thePalindrome |
https://github.com/minetest/minetest/blob/master/src/util/auth.cpp#L24 |
| 19:30 |
thePalindrome |
Note: I believe you, I'm just trying to figure the code out here :P |
| 19:30 |
hmmmm |
auth.cpp implements the legacy password and provides an abstraction layer between the various authentication methods |
| 19:30 |
hmmmm |
legacy password system |
| 19:30 |
thePalindrome |
Oh duh :P |
| 19:31 |
hmmmm |
because the way it was implemented, srp was made into its own library that est31 maintains on his own |
| 19:31 |
thePalindrome |
I see |
| 19:31 |
hmmmm |
and the legacy auth system remains in auth.cpp because it's so small it doesn't deserve its own source file |
| 19:31 |
thePalindrome |
the wiki *really* needs updating then |
| 19:31 |
hmmmm |
nobody uses the wiki |
| 19:31 |
* thePalindrome |
does |
| 19:39 |
|
turtleman joined #minetest-dev |
| 19:40 |
|
Taoki joined #minetest-dev |
| 19:40 |
twoelk |
obviosly those that know all the secrets of minetest have invested a lot of hard work and much time in understanding their little corner of the code. This has cost way too much sweat, blood and tears to be shared with any wanabee script kid that stumbles through some wiki. Documenting one's knowledge is one of the hardest things to do anyways besides being abysmally boring and draining and unrewar |
| 19:41 |
thePalindrome |
lol |
| 19:41 |
thePalindrome |
At least I have an accepted patch :P |
| 20:52 |
|
Manzano_ joined #minetest-dev |
| 21:03 |
|
Hunterz1 joined #minetest-dev |
| 21:06 |
|
Taoki joined #minetest-dev |
| 21:12 |
|
Foghrye4__ joined #minetest-dev |
| 21:19 |
|
red-001 joined #minetest-dev |
| 21:24 |
|
FirePowi joined #minetest-dev |
| 21:32 |
|
thePalindrome joined #minetest-dev |
| 21:35 |
|
troller joined #minetest-dev |
| 21:36 |
sofar |
in my experience, wiki's function best if developers aren't asked to maintain them |
| 21:37 |
sofar |
if developers have to maintain them, it's a sure sign that the community doesn't have enough support to let developers code and people tracking what is happening in the code and update the wiki for them |
| 21:39 |
sofar |
fortunately we have #minetest-project :) |
| 21:42 |
|
Player_2 joined #minetest-dev |
| 21:49 |
|
hmmmmm joined #minetest-dev |
| 22:30 |
|
twoelk joined #minetest-dev |
| 22:39 |
|
Gael-de-Sailly joined #minetest-dev |
| 23:24 |
|
twoelk|2 joined #minetest-dev |
| 23:28 |
|
Samson1 joined #minetest-dev |
| 23:33 |
|
AnotherBrick joined #minetest-dev |
| 23:45 |
|
werwerwer joined #minetest-dev |
| 23:48 |
|
DI3HARD139 joined #minetest-dev |
