Page MenuHomeWildfire Games

phosit
User

Projects

User Details

User Since
Mar 25 2022, 1:22 PM (26 w, 5 d)

Recent Activity

Yesterday

phosit added a comment to D4770: Multiplayer saved games.
In D4770#204058, @mbusy wrote:

@phosit mh weird... I have been using this patch both from a windows desktop and an unbutu desktop, and played with a friend using it... What configuration do you use? What crash message do you obtain? I'll try to replicate your crash. I also wondered if this could be due to the fact that the patch is based on a slightly older version of the project (a month ago or so), I'll try to apply it with a more recent version of the project

Wed, Sep 28, 3:22 PM
phosit raised a concern with rP26915: Fix text alignment handling of spaces around wrapping..

Valgrind reports uninitialized reads. m_EndsWithSpace is sometimes not initialized.

Wed, Sep 28, 10:41 AM

Sun, Sep 25

phosit added inline comments to D4790: Use atomic as buffer index in CProfiler2::ThreadStorage.
Sun, Sep 25, 9:00 PM
phosit requested review of D4790: Use atomic as buffer index in CProfiler2::ThreadStorage.
Sun, Sep 25, 7:44 PM
phosit added a comment to D4770: Multiplayer saved games.

It does not work for me: as soon as i hit Save as a host of a MP game the game crashes. Same for the Load Game button.

Sun, Sep 25, 3:38 PM
phosit added a comment to D4664: Non-local condition_variable.

I'm not satisfied with the "selecting priority" inside PopTask. The predicate is not trivial and both mutexes are owned at the same time. Do you think this are problems? Do you have a solution?
My ideas (non of them is perfect and i omited m_Kill):

Sun, Sep 25, 11:36 AM

Sat, Sep 24

phosit added inline comments to D3676: Removes explicit synchronizations from MapGenerator.
Sat, Sep 24, 4:18 PM

Fri, Sep 9

phosit requested review of D4786: A bit faster "state hash check".
Fri, Sep 9, 2:12 PM

Thu, Sep 8

phosit accepted D4779: Removes drawing via IDeviceCommandContext from CMinimap.
Thu, Sep 8, 8:00 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

change parameter order of PushTask
add context

Thu, Sep 8, 3:16 PM
phosit added a comment to D4425: Use the threadpool for texture conversion.
In D4425#203777, @Stan wrote:

Can't you make it the responsibility of the receiver to destroy objects instead of adding the extra overhead of a unique ptr?

Thu, Sep 8, 2:34 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

Revert to the one that worked

Thu, Sep 8, 2:04 PM

Wed, Sep 7

phosit added a comment to D4425: Use the threadpool for texture conversion.
In D4425#203777, @Stan wrote:

Can't you make it the responsibility of the receiver to destroy objects instead of adding the extra overhead of a unique ptr?

I guess an overkill solution could also be to have a destruction queue that's emptied when trying to push new tasks.

Wed, Sep 7, 9:59 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

fix the actual memory leak bug

Wed, Sep 7, 9:57 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

fix the memory leak

Wed, Sep 7, 8:08 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

apply suggestions from Stan

Wed, Sep 7, 5:59 PM
phosit added inline comments to D4425: Use the threadpool for texture conversion.
Wed, Sep 7, 1:47 PM

Tue, Sep 6

phosit added inline comments to D4425: Use the threadpool for texture conversion.
Tue, Sep 6, 11:10 PM
phosit updated the diff for D4425: Use the threadpool for texture conversion.

Use Future to comunicate the return value and to ensure the task is stoped in the destructor.

Tue, Sep 6, 11:04 PM
phosit commandeered D4425: Use the threadpool for texture conversion.
Tue, Sep 6, 10:36 PM

Mon, Sep 5

phosit added inline comments to rP22772: pthread -> std::thread (5/7) - Replace sdl semaphore with condition variable.
Mon, Sep 5, 1:26 PM

Sun, Sep 4

phosit added a comment to D4784: DummySoundManager.

A namespace can't have I prefix, because it's not an interface. As it doesn't provide a contract. Which means using namespace in such manner is much more error prone than using a real interface.

The name can also be changed.

Sun, Sep 4, 9:14 PM
phosit updated the diff for D4784: DummySoundManager.

I tried to only remove the down-casts.

Sun, Sep 4, 9:08 PM
phosit planned changes to D4784: DummySoundManager.
Sun, Sep 4, 8:09 PM
phosit requested review of D4784: DummySoundManager.
Sun, Sep 4, 7:33 PM
phosit added inline comments to D4677: span and string_view for CmdLineArgs.
Sun, Sep 4, 5:56 PM
phosit added inline comments to D4677: span and string_view for CmdLineArgs.
Sun, Sep 4, 1:21 PM

Fri, Sep 2

andy5995 awarded D4677: span and string_view for CmdLineArgs a Mountain of Wealth token.
Fri, Sep 2, 8:39 PM
phosit abandoned D4766: [WIP] PS::Execution.

I apologise to thous who put time in reviewing.

Fri, Sep 2, 7:29 PM
phosit added a comment to D4766: [WIP] PS::Execution.

For now we should put ower time in improving Future and TaskManager.
When we have coroutines we should again take a look at this interface. When all compilers support coroutines we can also use libunifex.

Fri, Sep 2, 7:24 PM
phosit abandoned D4682: Profiler2 lives in main().

On a second thought someone might expect the profiler to have a livetime like std::clog. Wen using CProfiler in static initialization there would be an dangling reference.
I think it would be bether to have components (HTTP / GPU) be handled by RTTI. Propably I do that in an other diff.

Fri, Sep 2, 7:15 PM

Wed, Aug 31

phosit added a comment to D4779: Removes drawing via IDeviceCommandContext from CMinimap.

The code in GUI is easier. But there is duplicate in Canvas2D.cpp. Could DrawRotatedTexture just compute the rotated destination and call DrawTexture

Wed, Aug 31, 4:56 PM

Aug 28 2022

phosit updated the diff for D4730: Type-erased SharedState.
Aug 28 2022, 5:37 PM
phosit added inline comments to D4730: Type-erased SharedState.
Aug 28 2022, 10:57 AM

Aug 27 2022

phosit added a comment to D4773: Normalize names for ClientId in /network.

The new naming makes sense.
Does anybody know why it was called m_HostId before?

Aug 27 2022, 6:46 PM
phosit added inline comments to D4774: Fixes space correction after rP26915.
Aug 27 2022, 12:38 PM
phosit accepted D4774: Fixes space correction after rP26915.

The code is reasonable ✓
The bug is fixed ✓
The test pass ✓, and don't pass if the actual fix is not applied ✓
I don't see any new text missalignement ✓

Aug 27 2022, 11:50 AM

Aug 25 2022

phosit added inline comments to D4602: Avoids assertions in case of corrupted game saves.
Aug 25 2022, 11:19 AM

Aug 24 2022

phosit added a comment to D4751: Fix the specialness of `FindTemplatesUnrestricted`..
In D4751#203259, @Stan wrote:

@phosit it seems one cannot cast from const_iterator aka _Array_const_iterator to T* any clue?

Well the C++ standard does not specify that const_iterator is convertible to T*.

Aug 24 2022, 5:14 PM

Aug 22 2022

phosit added inline comments to rP26915: Fix text alignment handling of spaces around wrapping..
Aug 22 2022, 9:39 PM
phosit added inline comments to D4662: Fix text alignment handling of spaces around wrapping.
Aug 22 2022, 9:31 PM
phosit added a comment to rP26915: Fix text alignment handling of spaces around wrapping..

What menu is that?

Aug 22 2022, 4:29 PM
phosit added inline comments to D4730: Type-erased SharedState.
Aug 22 2022, 11:04 AM

Aug 21 2022

phosit updated the summary of D4730: Type-erased SharedState.
Aug 21 2022, 7:25 PM
phosit updated the summary of D4730: Type-erased SharedState.
Aug 21 2022, 7:23 PM
phosit added inline comments to D4751: Fix the specialness of `FindTemplatesUnrestricted`..
Aug 21 2022, 6:10 PM
phosit added inline comments to D4772: Remove some unnecesary string copy. related to substr.
Aug 21 2022, 4:50 PM
phosit updated the diff for D4772: Remove some unnecesary string copy. related to substr.

CI failed

Aug 21 2022, 4:48 PM
phosit updated the diff for D4772: Remove some unnecesary string copy. related to substr.

fix windows build

Aug 21 2022, 2:58 PM
phosit added a comment to D4730: Type-erased SharedState.

We could go even further and remove PackagedTask. The TaskManager then would hold std::shared_ptr<ErasedSharedState>

Aug 21 2022, 2:48 PM
phosit updated the diff for D4772: Remove some unnecesary string copy. related to substr.

coments from Stan and copyright year

Aug 21 2022, 2:27 PM

Aug 20 2022

phosit added inline comments to D4772: Remove some unnecesary string copy. related to substr.
Aug 20 2022, 7:52 PM
phosit requested review of D4772: Remove some unnecesary string copy. related to substr.
Aug 20 2022, 5:11 PM
phosit added inline comments to D4770: Multiplayer saved games.
Aug 20 2022, 1:32 PM
phosit added a comment to D4766: [WIP] PS::Execution.

From a 40,000ft view, libraries like libunifex already implement similar sender-receiver functionality à la the wg21 P2300 paper. Are there particular advantages of adding our own implementation of a subset of p2300 vs. adopting a library like libunifex which offers a much broader set of functionality?

I don't think that we want to depend on another library. Libutifex requires a newer compiler than we do.
If libunifex gets more approval, I'm in. After all we also have {fmt}.

Aug 20 2022, 11:29 AM

Aug 19 2022

phosit updated the diff for D4730: Type-erased SharedState.
  • template Future on return type.
  • use std::optional
  • Future does not implicitly convert the result type into its stored-type. That was considered a feature.
Aug 19 2022, 9:09 PM
phosit added a comment to D4766: [WIP] PS::Execution.

An other arguments ;)
The syntax for recurrent tasks https://code.wildfiregames.com/D3877 would be easy: syncWait(taskManager.schedule() | task | repeatAfter(duration)).

Aug 19 2022, 9:41 AM

Aug 18 2022

phosit added a comment to D4768: Fixes TaskManager number of worker calculation.

And how well does your task manager work with zero threads?

Works fine as said above.

Sory that was only a rhetorical question. I thaught it would not work. seems task-cancelation works great.

Aug 18 2022, 6:36 PM
phosit updated the diff for D4764: vector of locale in L10n.
  • Free function -> make m_AvailableLocales a const member
  • Rename member variables
  • Return const reference from GetCurrentLocale
Aug 18 2022, 6:31 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.
In D4768#202987, @Stan wrote:

numberOfWorkers < MIN_THREADS is perfectly fine.

Well it is only because of the cassert you added preventing MIN_THREADS to be 0. But then you have a logic flaw. If it can be lower than MIN_THREADS then MIN_THREADS is not the minimum bound and its name is incorrect.

Currently the name is also not perfect. It Should be MIN_WORKER.
Pyrogenesis uses between MIN_THREADS and MAX_THREADS threads. If the sound manager and userreporter and... would use TaskManager. :)

Aug 18 2022, 4:43 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.
static_assert(MIN_THREADS > 0 && MIN_THREADS <= MAX_THREADS);
numberOfWorkers = Clamp(std::thread::hardware_concurrency(), MIN_THREADS, MAX_THREADS) - 1;

As pointed by Stan it's wrong.

No Stan also didn't understand me. I think i'm the problem :(
numberOfWorkers < MIN_THREADS is perfectly fine.

Aug 18 2022, 4:16 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.
In D4768#202983, @Stan wrote:

Should be:

cassert(MIN_THREADS > 0 && MIN_THREADS <= MAX_THREADS);
numberOfWorkers = Clamp(std::thread::hardware_concurrency() - 1, MIN_THREADS, MAX_THREADS);

Else you can get numberOfWorkers < MIN_THREADS

Thats the way it is implemented right now.
MIN_THREADS; MAX_THREADS and hardware_concurrency() also counts the main thread.

Aug 18 2022, 3:58 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.

You suggest:

size_t GetDefaultNumberOfWorkers()
{
	const size_t hardware_concurrency = std::thread::hardware_concurrency();
	return hardware_concurrency ? hardware_concurrency - 1 : 0;
}
Aug 18 2022, 3:43 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.

You can set both MIN_THREADS and MAX_THREADS to zero and it will work perfectly fine.

That's not different with your patch.

Aug 18 2022, 2:38 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.

That's a not good fix, as it implicitly relies on MIN_THREADS non being zero.

Aug 18 2022, 1:57 PM
phosit added a comment to D4768: Fixes TaskManager number of worker calculation.

Why do we need an extra function GetDefaultNumberOfWorkers? Evaluating the number of workers is done in the constructor.
The problem would be solved by: Clamp<size_t>(std::thread::hardware_concurrency() - 1, MIN_THREADS, MAX_THREADS) -> Clamp<size_t>(std::thread::hardware_concurrency(), MIN_THREADS, MAX_THREADS) - 1. The the equation "Worker + main = Threads" would also make sens.

Aug 18 2022, 12:59 PM
phosit added a comment to D4764: vector of locale in L10n.

availableLocales is assigned only in the constructor. If LoadListOfAvailableLocales would be a free or static function availableLocales could be initialized in the constructor and be const

Aug 18 2022, 12:42 PM

Aug 16 2022

phosit added inline comments to D4764: vector of locale in L10n.
Aug 16 2022, 6:54 PM
phosit added a comment to D4764: vector of locale in L10n.

icu::Locale is about 200 bytes, if there is no too much vector resizing then changes make sense.

Aug 16 2022, 6:31 PM
phosit added a comment to D4766: [WIP] PS::Execution.

Gotta have to check those includes, lots are missing.

I deliberately omitted thous wich are already included in Execution.h. I'll add them in the next diff

Aug 16 2022, 6:20 PM
phosit requested review of D4766: [WIP] PS::Execution.
Aug 16 2022, 4:20 PM

Aug 15 2022

phosit requested review of D4764: vector of locale in L10n.
Aug 15 2022, 5:40 PM
phosit added inline comments to D4705: Remove sys_generate_random_bytes.
Aug 15 2022, 3:11 PM
phosit added inline comments to D4705: Remove sys_generate_random_bytes.
Aug 15 2022, 2:50 PM

Aug 14 2022

phosit added a comment to D4761: remove cassert.

I don't think the condition is useful without context. But i'm ok if i should leave it that way.

Aug 14 2022, 10:38 PM
phosit added a comment to D4761: remove cassert.

Have you checked MSVC output in case of an error?

Aug 14 2022, 10:09 PM
phosit requested review of D4761: remove cassert.
Aug 14 2022, 3:51 PM

Aug 13 2022

phosit added a comment to D4743: Ignore broken symlinks.

Acording to https://trac.wildfiregames.com/wiki/Coding_Conventions#Errorreporting we should not use LOGWARNING:
"only use things defined in source/lib/"

Aug 13 2022, 7:05 PM
phosit abandoned D4759: UTF-8 as execution caracter set.

It is already UTF-8‽ So it should be possible to replace wchar_t by char in most places.

Aug 13 2022, 2:26 PM
phosit requested review of D4759: UTF-8 as execution caracter set.
Aug 13 2022, 1:56 PM
phosit added a comment to D4758: Rearrange GC scheduling for reduced lag.

Realy nice improvement... and less controversal than the allocator.

Aug 13 2022, 12:04 PM

Jul 28 2022

phosit added inline comments to D4718: Allocator and generator for component data..
Jul 28 2022, 11:28 AM

Jul 27 2022

phosit added a comment to D4743: Ignore broken symlinks.

Acording to https://trac.wildfiregames.com/wiki/Coding_Conventions#Errorreporting we should not use LOGWARNING:
"only use things defined in source/lib/"

Jul 27 2022, 1:54 PM

Jul 26 2022

phosit updated the diff for D4743: Ignore broken symlinks.
Jul 26 2022, 7:27 PM
phosit added inline comments to D4718: Allocator and generator for component data..
Jul 26 2022, 5:41 PM
phosit added inline comments to D4718: Allocator and generator for component data..
Jul 26 2022, 11:46 AM

Jul 25 2022

phosit added a comment to D4743: Ignore broken symlinks.
In D4743#201740, @Stan wrote:

Might try filesystem if that works. I don't remember if nothing worked, or only a subset.

Jul 25 2022, 10:33 PM
phosit added a comment to D4743: Ignore broken symlinks.
In D4743#201732, @Stan wrote:

They trigger Assertion popups. You're free to ignore suppress them. Except in Atlas where it goes south and crashes the entire game.

Jul 25 2022, 9:49 PM
phosit updated the diff for D4743: Ignore broken symlinks.

Handle error also on windows (hopefully i can't test ^^)

Jul 25 2022, 9:42 PM
phosit added a comment to D4743: Ignore broken symlinks.

Why do WARN_ON_ERROR and WARN_RETURN abort the program? shouldn't they warn as the name says?

Jul 25 2022, 9:04 PM
phosit added inline comments to D4743: Ignore broken symlinks.
Jul 25 2022, 12:06 PM
phosit added a comment to D4743: Ignore broken symlinks.

Before applying the patch I would get the following output

Terminal output
❯ pyrogenesis
TIMER| InitVfs: 2.513 ms
FILES| Main log written to '/Users/paria/Library/Application Support/0ad/logs/mainlog.html'
FILES| Interesting log written to '/Users/paria/Library/Application Support/0ad/logs/interestinglog.html'
TIMER| CONFIG_Init: 6.691 ms
Function call failed: return value was -110301 (Error during IO)
Location: file_system.cpp:123 (GetDirectoryEntries)

Call stack:

(error while dumping stack: Function not supported)
errno = 2 (Error during IO)
OS error = ?

I also got an "Error during IO" but why? it is not EIO?

Jul 25 2022, 12:02 PM

Jul 24 2022

phosit updated the diff for D4743: Ignore broken symlinks.

Include logger

Jul 24 2022, 10:35 PM
phosit requested review of D4743: Ignore broken symlinks.
Jul 24 2022, 10:20 PM
phosit requested review of D4739: Return terrain reference from CWorld.
Jul 24 2022, 1:36 PM

Jul 22 2022

phosit updated the diff for D4705: Remove sys_generate_random_bytes.

fix test

Jul 22 2022, 8:21 PM
phosit updated the diff for D4705: Remove sys_generate_random_bytes.
In D4705#201569, @Stan wrote:

Header date, the test has been lost,

Jul 22 2022, 7:13 PM
phosit added a comment to D4735: Move all buildings to the builder mixin.

In most civs this looks weird: wall; tower; tower; wall; tower
IMO we shouldn't sort by phase.

Could you expand on that / how would you sort it ?
Rather by function e.g. eco buildings p1, eco buildings p2, military buildings p1 .... Ect?

Jul 22 2022, 7:06 PM
phosit updated the diff for D4705: Remove sys_generate_random_bytes.
  • braces
  • function names
  • CStr

Did i miss something?

Jul 22 2022, 3:56 PM
phosit added inline comments to D4705: Remove sys_generate_random_bytes.
Jul 22 2022, 3:29 PM