Implement load_g1() #20

rwjuk · 2018-01-21T17:38:51Z

~~Initial decompilation of the G1.DAT load function - doesn't work because various offset fixes are TBD. Please feel free to fix up.~~ load_g1() implemented.

IntelOrca · 2018-01-21T17:46:18Z

src/openloco/graphics/gfx.cpp

+            if (fread(&header, 8, 1, file) == 1) {
+
+                // Read element headers
+                g1Elements = (loco_g1_element *)malloc(header.num_entries * sizeof(loco_g1_header));


You won't be able to do this until all references to g1Elements are implemented or replaced.
You will need to set it or copy it to 0x9E2424

Ah yeah, of course. Guessing we should have a define for that, akin to RCT2_ADDRESS_G1_ELEMENTS?

Yes, loco_global_array<loco_g1_element, 15000, 0x9E2424> _g1Elements etc.

marijnvdwerf · 2018-01-21T21:11:52Z

src/openloco/graphics/gfx.cpp

+
+        file = fopen(g1Path.make_preferred().u8string().c_str(), "rb");
+
+        if (file != NULL) {


Try to add some early escapes, to prevent nesting too much.

marijnvdwerf · 2018-01-21T21:34:23Z

src/openloco/graphics/gfx.h

+        uint32_t total_size;
+    };
+
+     struct loco_g1_element


g1_element_t seems to be more consistent with the rest of the codebase

I'll change it - I just followed OpenRCT2's rct_g1_element.

So far I have used _t for any time that has no default initialisation. E.g. it won't be zeroed if you define a local instance of one. And it won't have any methods etc.

janisozaur · 2018-01-21T22:16:10Z

src/openloco/graphics/gfx.cpp

+                g1Buffer = malloc(header.total_size);
+                fread(g1Buffer, header.total_size, 1, file);
+
+                fclose(file);


you close it twice

janisozaur · 2018-01-21T22:17:41Z

src/openloco/graphics/gfx.cpp

+
+                // Read element data
+                g1Buffer = malloc(header.total_size);
+                fread(g1Buffer, header.total_size, 1, file);


This needs return value checked. Since you already throw in case there is no g1.DAT, you could maybe add a try { … } catch block here (encompassing both freads), upon exception close the file and re-throw?

Hmm... good point - I'll do just that I think.

IntelOrca · 2018-01-21T22:53:22Z

We should probably use iostream as that is the C++ way.

AaronVanGeffen · 2018-01-21T23:29:58Z

src/openloco/graphics/gfx.h

@@ -4,6 +4,8 @@
 #include "../localisation/stringmgr.h"
 #include "../openloco.h"

+#define LOCO_G1_ELEMENT_COUNT 0x1A10


Does this hold for the other variants (Steam/GOG/…) as well?

GOG, yes. Steam... no, it's 0x380F. I'll need to investigate that.

Are you sure it's not 0x101A and 0xF38? Anyhow, I'd say log a warning in case g1 count doesn't match expected count. Probably best to still use the other one for array size, in case something depends on it.

IntelOrca · 2018-01-22T08:01:06Z

src/openloco/graphics/gfx.cpp

-        call(0x0044733C);
+        auto g1Path = environment::get_path(environment::path_id::g1);
+
+        std::ifstream stream(g1Path.make_preferred().u8string().c_str(), std::ios::in | std::ios::binary);


ifstream takes a std::path, so you should just be able to do:
std::ifstream stream(g1Path, std::ios::in | std::ios::binary);

Yep, victim of copy-paste from fopen - will fix.

rwjuk · 2018-01-23T19:06:09Z

Exception thrown at 0x7709FD1A (ntdll.dll) in openloco.exe: 0xC0000005: Access violation reading location 0x00000004.. Line 117 when copying the elements vector into _g1Elements.

janisozaur · 2018-01-23T19:47:59Z

src/openloco/graphics/gfx.cpp

@@ -98,6 +98,8 @@ namespace openloco::gfx
        // This code copies the closest variants into their place, and moves other elements accordingly
        if (header.num_entries == g1_expected_count::steam)
        {
+            elements.reserve(g1_expected_count::disc);


resize(). reserve() will only change capacity so you can pre-emptively allocate the space, but vector's size stays the same and the objects you copy in will not survive any calls that modify (e.g. copying to another vector) or will be replaced with values pushed in future.

Yes, sorry I didn't realise it needed to grow.

marijnvdwerf · 2018-01-24T11:43:38Z

src/openloco/graphics/gfx.cpp

+
+            // Extra two tutorial images
+            std::copy_n(&elements[3549], header.num_entries - 3549, &elements[3551]);
+            std::copy_n(&elements[3551], 1, &elements[3549]);


Shouldn't this be 3550?

Yes, and I don't think we need L106 after all.

IntelOrca · 2018-01-25T13:15:17Z

src/openloco/graphics/gfx.h

@@ -18,11 +18,55 @@ namespace openloco::gfx
        int16_t pitch;       // 0x0C note: this is actually (pitch - width)
        uint16_t zoom_level; // 0x0E
    };
-
 #pragma pack(pop)


Needs removing

Sorry, must've accidentally reintroduced it during rebase.

rwjuk added the work in progress label Jan 21, 2018

IntelOrca reviewed Jan 21, 2018

View reviewed changes

rwjuk force-pushed the g1attempt branch from 3c73328 to afe4ae8 Compare January 21, 2018 20:45

rwjuk removed the work in progress label Jan 21, 2018

rwjuk changed the title ~~Initial work on g1.dat load~~ Implement load_g1() Jan 21, 2018

marijnvdwerf reviewed Jan 21, 2018

View reviewed changes

janisozaur reviewed Jan 21, 2018

View reviewed changes

rwjuk force-pushed the g1attempt branch from 8a5ff8c to cd609b1 Compare January 21, 2018 22:52

rwjuk force-pushed the g1attempt branch from cd609b1 to 1c8c3b4 Compare January 21, 2018 22:54

AaronVanGeffen reviewed Jan 21, 2018

View reviewed changes

rwjuk force-pushed the g1attempt branch from a9b6d8d to 8dc33ea Compare January 22, 2018 01:08

IntelOrca reviewed Jan 22, 2018

View reviewed changes

rwjuk force-pushed the g1attempt branch from 8dc33ea to 24a3e7a Compare January 23, 2018 00:12

IntelOrca force-pushed the g1attempt branch from 24a3e7a to ed9b2ca Compare January 23, 2018 11:55

janisozaur reviewed Jan 23, 2018

View reviewed changes

marijnvdwerf reviewed Jan 24, 2018

View reviewed changes

rwjuk and others added 10 commits January 25, 2018 12:48

Implement load_g1()

48e9c26

Throw a runtime error for invalid g1.DAT

3d8f4e0

Tidy load_g1()

6a5fc61

Use ifstream for file reads

1089534

Fix read call

e6a05fd

Automatically fixup Steam G1.DAT

f863087

Refactor load_g1 to be more C++y

365a85a

Add 64-bit compatibility, future proof

547de40

Add stream helpers

cdd794c

Move and refactor g1 constants

3e1aea9

rwjuk added 2 commits January 25, 2018 12:54

Reserve elements in vector before copying

87ad512

resize rather than reserve vector

24e43c3

rwjuk force-pushed the g1attempt branch from 5738229 to 077c9d9 Compare January 25, 2018 12:55

IntelOrca reviewed Jan 25, 2018

View reviewed changes

Correct steam g1.dat fixup indices

ad4d894

rwjuk force-pushed the g1attempt branch from 54b81d4 to ad4d894 Compare January 25, 2018 14:15

IntelOrca approved these changes Jan 25, 2018

View reviewed changes

AaronVanGeffen mentioned this pull request Jan 25, 2018

Game graphic broken #52

Closed

IntelOrca added 2 commits January 25, 2018 17:20

Add default constructor to g1_element

4624ad0

Format code

0a67893

IntelOrca merged commit d768909 into OpenLoco:master Jan 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement load_g1() #20

Implement load_g1() #20

rwjuk commented Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

rwjuk Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

marijnvdwerf Jan 21, 2018

marijnvdwerf Jan 21, 2018

rwjuk Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

janisozaur Jan 21, 2018

janisozaur Jan 21, 2018 •

edited

rwjuk Jan 21, 2018

IntelOrca commented Jan 21, 2018

AaronVanGeffen Jan 21, 2018

rwjuk Jan 21, 2018

marijnvdwerf Jan 22, 2018

IntelOrca Jan 22, 2018

rwjuk Jan 22, 2018

rwjuk Jan 23, 2018

rwjuk commented Jan 23, 2018

janisozaur Jan 23, 2018

IntelOrca Jan 24, 2018

marijnvdwerf Jan 24, 2018

rwjuk Jan 25, 2018

IntelOrca Jan 25, 2018

rwjuk Jan 25, 2018


		file = fopen(g1Path.make_preferred().u8string().c_str(), "rb");

		if (file != NULL) {

Implement load_g1() #20

Implement load_g1() #20

Conversation

rwjuk commented Jan 21, 2018 • edited

IntelOrca Jan 21, 2018 • edited

Choose a reason for hiding this comment

rwjuk Jan 21, 2018 • edited

Choose a reason for hiding this comment

IntelOrca Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjuk Jan 21, 2018 • edited

Choose a reason for hiding this comment

IntelOrca Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janisozaur Jan 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IntelOrca commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjuk commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjuk commented Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

rwjuk Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

rwjuk Jan 21, 2018 •

edited

IntelOrca Jan 21, 2018 •

edited

janisozaur Jan 21, 2018 •

edited