bgbennyboy / Dinky-Explorer

An explorer/viewer/dumper tool for games using the Dinky engine. That's Return to Monkey Island, Thimbleweed Park and Delores.
35 stars 3 forks source link

Support for older Thimbleweed Park versions #1

Closed NorTreblig closed 6 years ago

NorTreblig commented 6 years ago

I've made some minor fixes + added support for older Thimbleweed Park versions (namely versions before v1421.957 till the very first official release v1296.849).

Method BundleReader_ggpack.DetectBundle() now tries to automatically detect the file version and method DecodeUnbreakableXor() acts accordingly.

NorTreblig commented 6 years ago

I found a difference in output from this tool to the one I am using: The last entry in .ggpack2 was missing and the second to last one contained data from both files. Added a fix.

bgbennyboy commented 6 years ago

Thanks for these I'm just looking through them now :)

The only one I'm not sure about is https://github.com/bgbennyboy/Thimbleweed-Park-Explorer/pull/1/commits/8e5a4c5d7bbe05387f019f1594031806cc2b6f84

I dont understand the need for the extra try/catches in the constructor. Wont the filestream be disposed of anyway when the destructor runs? I also dont fully understand the try/catch/throw - lines 79-84. Is Dispose likely to raise an exception?

This isn't a criticism - this is the first C# program I've written in 11 years so if its a common c# pattern please correct me. Again thank you very much for the fixes :)

NorTreblig commented 6 years ago

This isn't a criticism

No problem :-)

try/catch/throw - lines 79-84. Is Dispose likely to raise an exception?

Dispose methods normally shouldn't throw exceptions but of course it can happen. It's calling our own code and there could be bugs in the cleanup, like NullReferenceExceptions or concurrency issues.

You want to swallow exception in such case because you don't want to mask the original exception which is the one we need to see and handle. (Otherwise you are right: typically you never want to just swallow exceptions like this.)

constructor. Wont the filestream be disposed of anyway when the destructor runs?

Yes. WHEN the destructor runs. Memory is managed by a garbage collector and only when a garbage collection occurs and it's decided to remove this object it will be fully disposed and the destructor/finalizer will be called.

This is where the IDisposable pattern comes into play: You use it to make it clear that there should happen cleanup as soon as possible when this objects goes out of scope. Usually you use using-blocks in C# when possible to automatically call Dispose methods right away (which are also called in case of exceptions in this block).

In this case a Stream is used which implements IDisposable and should be cleaned up accordingly. File streams in particular use file handles (i.e. unmanaged resources) and if you don't clean them up they stay around until garbage collection decides to do it (which can be as late when the application closes). You can just try it: Open an invalid file with the old code and then open the same file again -> file can not be accessed because previous file stream wasn't disposed/closed.

Back to our constructor: When the constructor fails there is actually no object returned which can be used with the IDisposable pattern. So when constructors create disposable objects it needs to (well, should) clean them up themselves in case of failure.

bgbennyboy commented 6 years ago

Thank you, thats a great explanation.

bgbennyboy commented 6 years ago

Doing a new release now with your changes. Many thanks :)