Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crude initial port to macOS. #3

Merged
merged 12 commits into from
Apr 4, 2020

Conversation

oluseyi
Copy link
Contributor

@oluseyi oluseyi commented Mar 30, 2020

Compiles, but misparses PSD file header upon executing PsdSamples.

Path to sample PSD currently hard-coded, because Xcode build locations are complex ;-)

I'll spend some time this week debugging the core flow, and then raise some issues around design ergonomics. There are a few places in NativeFile where Windows-centric assumptions have constrained the API.

…on executing PsdSamples.

Path to sample PSD currently hard-coded, because Xcode build locations are complex ;-)
@oluseyi oluseyi mentioned this pull request Mar 30, 2020
@MolecularMatters
Copy link
Owner

Thank you for this.

Two minor things:

  • Please make sure to follow the coding style of the rest of the code, e.g. put opening braces on the next line.

  • Please rename platform-specific files to *_Mac (uppercase M) to be consistent with the naming conventions. I'll rename the Windows-specific files accordingly.

One major thing:
The code seems to ignore individual file operations and just waits for any async. IO operation to be completed in DoWaitForRead and DoWaitForWrite.
Surely there must be a way to wait for individual async. operations on OSX?

What happens when starting two async. reads, e.g.
File::ReadOperation op1 = file->Read(...);
File::ReadOperation op2 = file->Read(...);
WaitForRead(op1);
WaitForRead(op2);

WaitForRead(op1) should only wait until the first operation has finished.

@oluseyi
Copy link
Contributor Author

oluseyi commented Mar 30, 2020

Thank you! These are exactly the sorts of guidances I wanted to understand.

Style guidelines, no problem. I'll push a commit to fix those later today.

Async, I was thinking about this overnight and realized that if I use the MemoryAllocator, I can defer the read operation until DoWaitForRead is called. That should solve this. Might take a day for me to get around to finishing that.

@oluseyi
Copy link
Contributor Author

oluseyi commented Apr 1, 2020

So @MolecularMatters, I'm having a challenging time getting some Mac-specific constructs to play nicely with the existing APIs. I tried to use the Allocator passed into PsdNativeFile to create instances of some structs I'd created to hold operation attributes (I switched to relying on Grand Central Dispatch, which exposes a C API), but it appears that the Cocoa-based nature of some of these constructs fails the util::IsPod assertion.

What are your thoughts on dynamically allocating the memory outside of the passed-in Allocator, or are there other methods I can use?

The types:

typedef void (^DispatchIOHandler)(dispatch_data_t data, int error);

struct DispatchReadOperation
{
    void* dataReadBuffer;
    uint32_t length;
    uint64_t offset;
    DispatchIOHandler ioHandler;
    dispatch_semaphore_t semaphore;
};

struct DispatchWriteOperation
{
    dispatch_data_t dataToWrite;
    size_t bytesWritten;
    uint32_t length;
    uint64_t offset;
    DispatchIOHandler ioHandler;
    dispatch_semaphore_t semaphore;
};

And the allocation attempts look like this:

DispatchReadOperation *operation = memoryUtil::Allocate<DispatchReadOperation>(m_allocator);
...
DispatchWriteOperation *operation = memoryUtil::Allocate<DispatchWriteOperation>(m_allocator);

@MolecularMatters
Copy link
Owner

Your new code looks good.
I'll relax the util::IsPod requirements and add support for non-PODs today.

@MolecularMatters
Copy link
Owner

I implemented the necessary changes in this commit:
83a12e0

@oluseyi
Copy link
Contributor Author

oluseyi commented Apr 1, 2020

Thank you. I'll merge that in and try to wrap up this initial port.

@oluseyi
Copy link
Contributor Author

oluseyi commented Apr 1, 2020

Hey @MolecularMatters:

  1. I've gotten SampleReadPsd() to work!

  2. I'm running into a challenge with SampleWritePsd() that I suspect has to do with Catalina's security permissions.

  3. Separately, I have to figure out how to implement NativeFile::DoGetSize(), given that I only hold the file descriptor. Ideas welcome!

Almost there…

@oluseyi
Copy link
Contributor Author

oluseyi commented Apr 1, 2020

The SampleWritePsd() issue might be related to file path config, which I wanted to talk about. There's the fact that POSIX systems use a different path separator than the Windows default, though I believe Windows APIs will do the right thing if you supply them with forward slash-separated paths? So perhaps we should standardize on that?

More fundamentally, Xcode is pretty weird about the working directory of your executable in debug, building it somewhere deep inside of the developer directory hierarchy. Using relative paths to locate files is thus a bit of a problem. Perhaps a configuration file of some sort would help here? We could have per-platform config files, each plain text, which the executable will read and parse on launch to know where to find the input sample PSD, and where to write the output as well (they need not be the same path).

I can write up a first pass, but I think I'd want to break that out into its own issue and PR. That might make this story blocked by that one, but I think it'd be the right call.

@MolecularMatters
Copy link
Owner

I added new code that should make it easy for you to add macOS-specific paths for the samples' output data:
4bbd743

@oluseyi
Copy link
Contributor Author

oluseyi commented Apr 4, 2020

Turns out I can use the same paths now, since we're using the same separator. Thank you!

There's no easy solve for the relative path issue; best I can recommend is adding a portion to the ReadMe that explains how to set a custom working directory when debugging under macOS. As for building from the command line (via xcodebuild), that introduces additional directories to represent the selected build scheme… »le sigh«.

I'll add a ReadMe update, and then consider the branch ready to merge.

Please consider this branch ready to merge, @MolecularMatters. Thank you.


I'll leave the full ReadMe update to indicate the existence of macOS support up to you. Please include the following:

macOS

When building and debugging the PsdSamples command line utility under Xcode, you will need to edit the current scheme's working directory so that it can locate the supplied PSD file. Set the working directory to be the build/Xcode directory inside of wherever you have checked out the psd_sdk source code.

Running it directly from the command line is a little trickier; copy the binary from whatever output location it is in to build/Xcode, and then execute.

@MolecularMatters MolecularMatters merged commit ef2006a into MolecularMatters:master Apr 4, 2020
@MolecularMatters
Copy link
Owner

Thank you for your work, I'll update the ReadMe accordingly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants